lib: Deprecate overrideScope in lieu of overrideScope' taking arguments in the conventional order
#47238
Conversation
Ericson2314
force-pushed
the
overrideScope-order
branch
from
53f9f83
to
e95589b
Compare
Ericson2314
force-pushed
the
overrideScope-order
branch
from
e95589b
to
658a3b9
Compare
| <varname>deepin</varname>. If you use any of these package sets and | ||
| experience problems, please check your local configuration for any | ||
| overrides / overlays and make sure they are now in the conventional | ||
| <literal>self: super: { … }</literal> form. |
There was a problem hiding this comment.
Line breaks look good!
This buries the most important user-facing change under quite a lot of technical bits that aren't relevant to the user trying to fix their system. Ideally we'd swap it: call out exactly who is impacted in the first few words, then how to fix it, then the technical why details.
There was a problem hiding this comment.
How do you like the order now?
Ericson2314
force-pushed
the
overrideScope-order
branch
from
658a3b9
to
5b9cf0c
Compare
This comment has been minimized.
This comment has been minimized.
|
As said elsewhere, I think this is a good change and I approve, without making a judgment call on whether it's 18.09 material. :) |
Ericson2314
force-pushed
the
overrideScope-order
branch
2 times, most recently
from
ea1a01e
to
d72801a
Compare
|
I don't think breaking people's Nix expressions just for "consistency" is a good idea, especially since the resulting error messages will likely be baffling. It also makes it impossible to share a Nix expression using |
|
I agree with @edolstra on this If we want consistency regardless, we can leave the old function as it is and mark it deprecated, while introducing a differently named new one with the changed argument order. |
|
Sure will do. Then we can send to 18.09 too without qualms. |
Ericson2314
force-pushed
the
overrideScope-order
branch
from
d72801a
to
a6e1079
Compare
Ericson2314
changed the title
overrideScope take arguments in the conventional orderoverrideScope in lieu of overrideScope' taking arguments in the conventional order
…order
The `overrideScope` bound by `makeScope` (via special `callPackage`)
took an override in the form `super: self { … }`. But this is
dangerously close to the `self: super { … }` form used by *everything*
else, even other definitions of `overrideScope`! Since that
implementation did not even share any code either until I changed it
recently in 3cf4354, this inconsistency
is almost certainly an oversight and not intentional.
Unfortunately, just as the inconstency is hard to debug if one just
assumes the conventional order, any sudden fix would break existing
overrides in the same hard-to-debug way. So instead of changing the
definition a new `overrideScope'` with the conventional order is added,
and old `overrideScope` deprecated with a warning saying to use
`overrideScope'` instead. That will hopefully get people to stop using
`overrideScope`, freeing our hand to change or remove it in the future.
Ericson2314
force-pushed
the
overrideScope-order
branch
from
a6e1079
to
b9dce11
Compare
|
I don't understand the warning message used here: Is it supposed say, "Do |
|
Yeah definitely, this was merged too soon imo, without proper review |
|
|
I wrote it markdown. I suppose |
Motivation for this change
Consistency with everything else. See the doc changes for details.
Since that implementation did not even share any code either until I changed it recently in 3cf4354, this inconsistency is almost certainly an oversight and not intentional.
There's no way to make this a smooth transition, sadly, but I hope in listing common uses in the manual, people searching for fixes to their problems will easy find it.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)