Skip to content

lib/attrsets: merge concatMapAttrs recursively#206965

Open
SuperSandro2000 wants to merge 1 commit intoNixOS:masterfrom
SuperSandro2000:concatMapAttrs-merge-recursive
Open

lib/attrsets: merge concatMapAttrs recursively#206965
SuperSandro2000 wants to merge 1 commit intoNixOS:masterfrom
SuperSandro2000:concatMapAttrs-merge-recursive

Conversation

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Dec 20, 2022

This makes it possible to drop name and just inherit values.

concatMapAttrsRecursive (name: value: { inherit (value) a b; }) { foo = { a = { q = null; }; b = { q = null;  }; bar = { a = { p = null;  }; b = { p= null; };};};}

Before the above only returned { a = { q = null; }; b = { q = null; }; } and with that change p is also included.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@SuperSandro2000 SuperSandro2000 requested review from figsoda, infinisil and roberth and removed request for edolstra, infinisil and nbp December 20, 2022 02:44
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 20, 2022
@figsoda
Copy link
Member

figsoda commented Dec 20, 2022

I would suggest having an additional concatMapAttrsRecursive function instead

@roberth
Copy link
Member

roberth commented Dec 20, 2022

Also skip ahead to the concatMapAttrsRecursiveUntil variant, because recursive merging/updating without knowing the meaning of the attrsets is bad.
For example, you might accidentally merge packages, where an attribute that's unique to the lower package ends up in the end result that's mostly the upper package. Good luck figuring out that bug.

@SuperSandro2000
lib/attrsets: add concatMapAttrsRecursive
a5d9627 
This makes it possible to drop name and just inherit values.

concatMapAttrsRecursive (name: value: { inherit (value) a b; }) { foo = { a = { q = null; }; b = { q = null; }; }; bar = { a = { p = null;  }; b = { p= null; };};}
@SuperSandro2000 SuperSandro2000 force-pushed the concatMapAttrs-merge-recursive branch from f8a1b85 to a5d9627 Compare December 22, 2022 02:27
@SuperSandro2000
Copy link
Member Author

SuperSandro2000 commented Dec 22, 2022

Added the recursive variant, don't really have the time to do a until variant right now.

figsoda
figsoda reviewed Dec 22, 2022
View reviewed changes
lib/attrsets.nix
/* Same as concatMapAttrs but merges the attrsets recurisvely.

Type:
concatMapAttrs ::
Copy link
Member

@figsoda figsoda Dec 22, 2022

Choose a reason for hiding this comment

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

Suggested change
concatMapAttrs ::
concatMapAttrsRecursive ::

@infinisil
Copy link
Member

infinisil commented Dec 22, 2022

What are the use cases for such a function?

@roberth
Copy link
Member

roberth commented Dec 22, 2022

I take back my previous comment. A truly recursive concatMapAttrs would not just merge one layer of values recursively, but merge each layer of values recursively. This is how I misunderstood this PR.

Instead of recursion, the new function seems to be about combining attribute values instead of picking one, so I think zipWithAttrs is the appropriate alternative to a "recursive" concatMapAttrs.
Perhaps this alternative could be documented in concatMapAttrs.

This was referenced Jun 14, 2023
Merged
Open
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@drupol drupol changed the title lib/attrsets: merge concatMapAttrs recurisvely lib/attrsets: merge concatMapAttrs recursively Sep 27, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 27, 2024
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Sep 27, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roberth roberth Awaiting requested review from roberth

@infinisil infinisil Awaiting requested review from infinisil

1 more reviewer

@figsoda figsoda figsoda left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@SuperSandro2000 @figsoda @roberth @infinisil @wegank