lib.customisation.makeScope: Make overrideScope consistent with makeScopeWithSplicing
#248988
Conversation
Artturin
changed the title
overrideScope consistent with `ma…overrideScope consistent with makeScopeWithSplicing
There was a problem hiding this comment.
Oof, the fact that overrideScope' didn't even exist on makeScopeWithSplicing is pretty bad, because depending on the set, only one of overrideScope or overrideScope' would work, and the other would warn or error, without a safe choice that always worked.
I'm generally okay with this change though
lib/customisation.nix
Outdated
| "`overrideScope` (from `lib.makeScope`) is deprecated. Do `overrideScope' (self: super: { … })` instead of `overrideScope (super: self: { … })`. All other overrides have the parameters in that order, including other definitions of `overrideScope`. This was the only definition violating the pattern." | ||
| (makeScope newScope (lib.fixedPoints.extends (lib.flip g) f)); | ||
| overrideScope = g: makeScope newScope (lib.fixedPoints.extends g f); | ||
| # Add a warning after 23.11 and remove after 24.05. |
There was a problem hiding this comment.
I think the timing should be much more relaxed. Somebody could be using overrideScope while still using 23.05 and get the warning that it's deprecated and overrideScope' should be used, but then they upgrade to 24.05 on the same day and suddenly .overrideScope' is gone, even though they only upgraded by 2 release.
There was a problem hiding this comment.
I meant remove it after 24.05 is branched off
There was a problem hiding this comment.
I'll tweak these, push coming soon.
There was a problem hiding this comment.
Agree. Looks like this was resolved in the latest push.
There was a problem hiding this comment.
Not really: #248988 (comment)
github-actions
bot
added
8.has: documentation
6.topic: emacs
6.topic: rust
6.topic: ocaml
labels
There was a problem hiding this comment.
LGTM. Looks like @infinisil's concern was addressed in the latest push.
lib/customisation.nix
Outdated
| overrideScope' = g: lib.warn | ||
| "`overrideScope'` (from `lib.makeScope`) has been renamed to `overrideScope`." |
There was a problem hiding this comment.
I needs to use warnIf (isInOldestRelease 2311), such that we only warn once 23.11 isn't supported anymore, otherwise we still have the problem from #248988 (comment), because there wouldn't be a transition period where users could use both versions of the function.
There was a problem hiding this comment.
My plan now is to backport the first commit to 23.05 so users can support stable and unstable at the same time
There was a problem hiding this comment.
I don't think that's a great idea, because then you can't just check whether a release is 23.05 to figure out if overrideScope is fine to use, instead you have to make sure it's 23.05 after a specific commit/date.
Using warnIf (isInOldestRelease ..) without a backport would be my preference, it means 23.11 will be a clear boundary of when it changed.
And anyways, there's no need to hurry with this, let's take it slow
lib/customisation.nix
Outdated
| (makeScope newScope (lib.fixedPoints.extends (lib.flip g) f)); | ||
| overrideScope' = g: makeScope newScope (lib.fixedPoints.extends g f); | ||
| overrideScope = g: makeScope newScope (lib.fixedPoints.extends g f); | ||
| # Remove after 24.05 is released. |
There was a problem hiding this comment.
| # Remove after 24.05 is released. |
Removal should definitely take a bunch longer! Otherwise somebody who's still used to using .overrideScope' updating from 23.11 to 24.11 would suddenly get an error!
Like yeah it can be removed at some point, but many people don't update to every NixOS release, and it doesn't cost us anything to leave the attribute in for some time.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…keScopeWithSplicing` Right now converting `makeScope` to `makeScopeWithSplicing` is not transparent to users and requires adding a warning for `overrideScope'` in the set itself. Warning and `overrideScope'` were added in 2018 b9dce11 and there should be no users left after 5 years.
`lib.makeScope` `overrideScope'` has been renamed to `overrideScope` `fd --type f | xargs sd --string-mode "overrideScope'" "overrideScope"`
|
Please don't self-merge when the PR is actively being reviewed and commented on.. (My approval happened at the exact same second as the merge, did you enable auto-merge?) |
Yes enabled auto merge |
|
I see, I think that's okay then, though I still think self-merges should be avoided when possible (in this case I would've merged it for you) |
|
Thanks for the reviews |
|
The auto-merge turns out to be only possible due to this recent change: #249117 (comment) |
|
Yeah, this was the first PR I saw it in and decided to try it. |
Right now converting
makeScopetomakeScopeWithSplicingis not transparent to users and requires adding a warning foroverrideScope'in the set itself.Warning and
overrideScope'were added in 2018 b9dce11 and there should be no users left after 5 years.Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)