-
-
Notifications
You must be signed in to change notification settings - Fork 17.5k
pocket-id: make overridable #399999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pocket-id: make overridable #399999
Conversation
1d22853 to
b475ba6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it by design that these attributes are still outside of finalAttrs scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're talking about the attributes in the let block, I'd like to keep them there, inherit them in the final derivation, and reference them with finalAttrs, I think it's more readable. I prefer not declaring those in the main derivation. If I missed your point, can you please clarify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if you put them into finalAttrs, when you have to change source version, you can easily override them with
pocket-id.overrideAttrs (prev: {
version = "0.49.0";
src = prev.src.overrideAttrs {
hash = "";
};
backend = prev.backend.overrideAttrs {
vendorHash = "";
};
frontend = prev.frontend.overrideAttrs {
npmDepsHash = "";
};
})So we don't have to copy definition of backend and frontend again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point now. When overriding nested attributes, eg. pocket-id.frontend.src, one should be able to do:
pocket-id.overrideAttrs (prev: {
src = prev.fetchFromGitHub {
hash = "";
};
})instead of:
pocket-id.overrideAttrs (prev: {
frontend = prev.frontend.overrideAttrs {
src = prev.fetchFromGitHub {
hash = "";
};
};
};Thanks for the idea, it's much simpler to use!
b475ba6 to
c07b1a9
Compare
marcusramberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! :)
Depends on #399817See #399817 (comment) as a use case
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.