python3.pkgs.beets: use fixed-point arguments#477084
python3.pkgs.beets: use fixed-point arguments#477084doronbehar merged 4 commits intoNixOS:masterfrom
Conversation
625d55a to
a146f50
Compare
9999years
left a comment
There was a problem hiding this comment.
Haven't done a thorough review but yay for finalAttrs! Maybe the pluginOverrides attr should be removed? I'd also like to see some more tests here...
I don't think so. It is still way more comfortable to configure built in plugins via |
I'm down, but testing what? |
Previously, the attribute set: python3.pkgs.beets-minimal.passthru.plugins.enabled filtered out the disabled plugins from: python3.pkgs.beets.passthru.plugins.all , and not from: python3.pkgs.beets-minimal.passthru.plugins.all . This meant that before the commit titled: python3.pkgs.beets: use fixed-point arguments , the derivations `beets` and `beets-minimal` were the same. Now this change has significance, were as in out of context PR NixOS#477088 it didn't.
9999years
left a comment
There was a problem hiding this comment.
Looks like this keeps the python3.pkgs.beets hash unchanged, which is a good sign!
I'm a bit nervous about the API for overriding plugins: with this PR, it's still impossible to use the (documented) pluginOverrides argument to override builtin plugins, as well as a whole host of attributes under passthru.plugins:
nix-repl> python3.pkgs.beets.passthru.plugins
{
all = { ... };
base = { ... };
builtins = { ... };
disabled = { };
disabledTestPaths = [ ];
enabled = { ... };
overrides = { };
wrapperBins = [ ... ];
}
To me, enabled, overrides, and all seem like reasonable places to put a new plugin, and it's not clear how base and builtins differ ... I'm not sure what a good solution for cleaning these up would be.
Perhaps we can just have a single passthru.plugins attribute and remove the rest? Not sure.
Anyways, I think this PR is a solid improvement over the status quo, but I'd like to get the plugin override API cleaned up soon.
24bbd41 to
a615e9d
Compare
Why not? I just added a test that proves it is possible..
I wouldn't say we wish to make it easy to override them all.
I agree, it is a remnant from the old implementation. I was thinking of removing them in #452540 but I wanted that PR to be easy to review. I'm open to simplify this but this would be a breaking change and I wish to not do that in this PR. |
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.