Skip to content
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

lib/recursiveUpdateUntil: fix code to match documentation #41604

Merged
merged 2 commits into from Aug 15, 2018
Merged

lib/recursiveUpdateUntil: fix code to match documentation #41604

merged 2 commits into from Aug 15, 2018

Conversation

bluescreen303
Copy link
Contributor

@bluescreen303 bluescreen303 commented Jun 6, 2018

Motivation for this change

The function recursiveUpdateUntil did not produce the expected result for the exact example given in the documentation.

This fixes that and makes the function useable for any predicate that actually looks at the path argument. In current nixpkgs, there is only 1 location that touches this functionality:

overrideConfig = thrower:
recursiveUpdateUntil (path: old: new:
path == thrower.path
) eval.config thrower.config;

I'm not fully sure what that is all about, but since it's maintainer-only I do not expect issues will arise because of this fix, as they probably just assumed this function worked as advertised as well.

$ nix repl lib
Welcome to Nix version 2.0.2. Type :? for help.

Loading 'lib'...
Added 350 variables.

-- this is the exact example from the function's documentation:
nix-repl> recursiveUpdateUntil (path: l: r: path == ["foo"]) {
                   # first attribute set
                   foo.bar = 1;
                   foo.baz = 2;
                   bar = 3;
                 } {
                   #second attribute set
                   foo.bar = 1;
                   foo.quz = 2;
                   baz = 4;
                 }
{ bar = 3; baz = 4; foo = { bar = 1; baz = 2; quz = 2; }; }

-- although the documentation says:
{
    foo.bar = 1; # 'foo.*' from the second set
    foo.quz = 2; #
    bar = 3;     # 'bar' from the first set
    baz = 4;     # 'baz' from the second set
}
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

$ nix repl lib
Welcome to Nix version 2.0.2. Type :? for help.

Loading 'lib'...
Added 350 variables.

-- this is the exact example from the function's documentation:
nix-repl> recursiveUpdateUntil (path: l: r: path == ["foo"]) {
                   # first attribute set
                   foo.bar = 1;
                   foo.baz = 2;
                   bar = 3;
                 } {
                   #second attribute set
                   foo.bar = 1;
                   foo.quz = 2;
                   baz = 4;
                 }
{ bar = 3; baz = 4; foo = { bar = 1; baz = 2; quz = 2; }; }

-- although the documentation says:
{
    foo.bar = 1; # 'foo.*' from the second set
    foo.quz = 2; #
    bar = 3;     # 'bar' from the first set
    baz = 4;     # 'baz' from the second set
}
@infinisil
Copy link
Member

Ping @Profpatsch

I'd rather have the documentation changed instead of the behaviour of the function. These functions might be widely used by other projects, which we can't know of whether it would break anything.

@Profpatsch
Copy link
Member

Profpatsch commented Jul 9, 2018

tbh that wouldn’t have happened if the original author spared five minutes to add the example to the library tests. 😞

I’m in favour of changing this function to match its documentation, because I don’t think we really need to be bug compatible in this case.

@bluescreen303 can you add a section to the release note file in nixos/doc/manual/release-notes/rl-1809.xml?
I’d like to see the example as a test in lib/tests/misc.nix as well.

@infinisil
Copy link
Member

Ping?

@Profpatsch
Copy link
Member

@infinisil added the test & release note, what do you say?

@infinisil
Copy link
Member

@GrahamcOfBorg eval

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Profpatsch Looking good

@Profpatsch Profpatsch merged commit d817452 into NixOS:master Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants