Skip to content

Comments

add: lib.foldlAttrs#218776

Merged
roberth merged 1 commit intoNixOS:masterfrom
hsjobeki:feature/lib-reduceAttrs
Mar 13, 2023
Merged

add: lib.foldlAttrs#218776
roberth merged 1 commit intoNixOS:masterfrom
hsjobeki:feature/lib-reduceAttrs

Conversation

@hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Feb 28, 2023 •
edited by roberth
Loading

Description of changes

Add new / missing lib function.

Handy shortcut for iterating over entries in an attrset and collecting the result in an accumulator.

Just like builtins.foldl', but for attrsets

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
  • Fits CONTRIBUTING.md.

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Feb 28, 2023 •
edited
Loading

@roberth can you tell me, if that lib function makes sense. If i should continue and add unit tests at ./lib/tests/misc.nix

Maybe i missed it and that functionality is already provided by some other function ? Although i didn't really find one, that's why i created this PR.

@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 Feb 28, 2023
roberth
roberth reviewed Feb 28, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

The general idea is good; just need to make it a bit more consistent with what we have.

@hsjobeki hsjobeki requested review from roberth and removed request for edolstra and infinisil March 2, 2023 11:47
hsjobeki
hsjobeki commented Mar 2, 2023
View reviewed changes
@hsjobeki hsjobeki changed the title add: lib.reduceAttrs add: lib.foldlAttrs Mar 3, 2023
hsjobeki
hsjobeki commented Mar 6, 2023
View reviewed changes
Copy link
Contributor Author

@hsjobeki hsjobeki left a comment

Choose a reason for hiding this comment

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

@roberth. Just added a unit test. Is there anything missing, or can we merge it?

@roberth
Copy link
Member

roberth commented Mar 6, 2023

You could think of corner cases and the behavior we would like there.
For example, an empty attrset doesn't need to evaluate the function.

foldlAttrs (throw "function not needed") 123 {}

The initial value is not needed when the function is constant. You could define last for attrsets this way, inefficiently.

lib.foldlAttrs (_acc: _name: v: v) (throw "accumulator not needed") { z = 3; a = 2; }

Also the accumulator doesn't have to be an attrset, and the initial value doesn't have to be "empty". The initial value goes first.

lib.foldlAttrs (acc: _name: v: acc * 10 + v) 1 { z = 3; a = 2; }

roberth
roberth reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Just some doc suggestions. Otherwise lgtm.

@hsjobeki hsjobeki force-pushed the feature/lib-reduceAttrs branch 2 times, most recently from 9a6d4cb to ff91c42 Compare March 10, 2023 19:55
@hsjobeki hsjobeki requested a review from roberth March 10, 2023 19:56
roberth
roberth reviewed Mar 10, 2023
View reviewed changes
roberth
roberth reviewed Mar 10, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

If you could squash the commits after fixing the type, I think this one is good to go!

@hsjobeki
init: lib.foldlAttrs
15a8d05 
- provide comprehensive example
- add unit test
@hsjobeki hsjobeki force-pushed the feature/lib-reduceAttrs branch from f081c4a to 15a8d05 Compare March 11, 2023 09:42
@hsjobeki hsjobeki requested a review from roberth March 13, 2023 09:31
@roberth roberth merged commit 4635079 into NixOS:master Mar 13, 2023
@roberth
Copy link
Member

roberth commented Mar 13, 2023

Thanks!

@Ma27
Copy link
Member

Ma27 commented Mar 13, 2023

Nice! That's how I always expected foldAttrs to behave ❤️ 🎉

@tie tie mentioned this pull request Mar 22, 2023
Closed
12 tasks
@infinisil infinisil mentioned this pull request Mar 23, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roberth roberth roberth approved these changes

Assignees

No one assigned

Labels

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.

3 participants

@hsjobeki @roberth @Ma27