beets: make it easier to override builtin plugins#471166
beets: make it easier to override builtin plugins#4711669999years wants to merge 1 commit intoNixOS:masterfrom
Conversation
|
Hey, thanks for the PR! Your approach on first sight seems better then mine: Could you give an example for an overlay using your change here? Indeed my overlay example seems pretty clumsy so I'd like to compare these overlays' clumsiness :). |
bc3a484 to
185e8e1
Compare
|
@doronbehar Thanks for the prompt! I added a couple of new Here's a usage example: (beets.override {
pluginOverrides = {
my_special_plugin.builtin = true;
};
}).overridePythonAttrs
(prev: {
postPatch = (prev.postPatch or "") + ''
mkdir -p beetsplug/my_special_plugin
touch beetsplug/my_special_plugin/__init__.py
'';
});I also added an example of building |
doronbehar
left a comment
There was a problem hiding this comment.
This approach is great. I have another nit request if you can please: Could you put the passthru.tests in 1 commit, and then fix beets.passthru.tests.beets-with-new-builtin-plugin in a 2nd commit? It'd be easier to see the fix more clearly. Also, in the 1st commit I'd put also a note in the big comment at the beginning of the file pointing out the examples in passthru.tests.
|
BTW On a 2nd thought, I think that my changes in #467091 don't contradict the changes here, and should be useful anyway. @9999years Could you review my changes there and then we can continue here? |
|
Oh and I almost forgot: The commit fixing |
185e8e1 to
587d99b
Compare
|
Sure, I'm going to split the |
This will make it possible to use `beetcamp` without setting `dontUsePythonCatchConflicts = true` in an override. See: NixOS#471166 (comment)
doronbehar
left a comment
There was a problem hiding this comment.
Changes look good. I think this should be good to go but I'd prefer to only approve it in the meantime so the other PRs could be coordinated.
587d99b to
ea4b924
Compare
This will make it possible to use `beetcamp` without setting `dontUsePythonCatchConflicts = true` in an override. See: NixOS#471166 (comment)
This will make it possible to use `beetcamp` without setting `dontUsePythonCatchConflicts = true` in an override. See: NixOS#471166 (comment)
|
Fixed merge conflicts. |
ea4b924 to
036fd2f
Compare
|
I forced pushed the same changes but with a different commit message, as now it is easier to override built in plugins - it was possible (but clumsy) thanks to my PR #467091 . |
doronbehar
left a comment
There was a problem hiding this comment.
Hmm the beets.passthru.tests.beets-with-new-builtin-plugin fail..
|
@9999years now since #271387 is now merged, I managed to create a working override test in #477084 , so I wish to close this. Is it OK with you? |
|
@doronbehar Once you have a PR which removes the |
|
Thanks for finding the time to comment :). We can discuss removing (or not) |
|
This PR is very small (17 lines) and still applicable; I've looked at this derivation a ton lately and I'm still not sure how to override builtin plugins without it. I'm not sure I understand the hesitancy to merge it / the rush to close it. The design in #477084 is likely better in the longer term (although there's a ton of Can we get this merged to fix that issue and then move on to #477084? I've been a little frustrated with you asking for changes and then insisting on merging your own PRs first (and having me rebase), even if they're more complex, and then finally closing my PR without merging it. I feel like I've wasted a lot of effort here and my initial issue ( |
I wouldn't have minded at all approving and merging this PR before #271387 got merged and I updated my PR. It's just that |
Previously, it required a `let in`: NixOS#467091
036fd2f to
fb900e5
Compare
|
OK. I think this is something of a "perfect is the enemy of the good" situation, and this PR would help fix a use-case today while we make sure #477084 is good to go, but we will have to agree to disagree. I do think that #477084 ( |
| let | ||
| beets-with-new-builtin-plugin = | ||
| (beets.override { | ||
| beets = beets-with-new-builtin-plugin; | ||
|
|
||
| pluginOverrides = { | ||
| my_special_plugin.builtin = true; | ||
| }; | ||
| }).overridePythonAttrs | ||
| (prev: { | ||
| postPatch = (prev.postPatch or "") + '' | ||
| mkdir -p beetsplug/my_special_plugin | ||
| touch beetsplug/my_special_plugin/__init__.py | ||
| ''; | ||
| }); | ||
| in | ||
| beets-with-new-builtin-plugin; |
There was a problem hiding this comment.
I see now how you fixed it! Pretty delicate indeed :).
Previously, it was possible to override builtin plugins. Now it's not. After this commit, it is again :)
You may have a patch (e.g. a not-yet-merged or not-yet-released PR) that adds a new builtin plugin.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.