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/deprecated: alias mapAttrsFlatten to mapAttrsToList #328381

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

tie
Copy link
Member

@tie tie commented Jul 19, 2024

Description of changes

These functions have identical implementation except for argument names.

This PR also updates usage in Nixpkgs.

mapAttrsFlatten:

# calls a function (f attr value ) for each record item. returns a list
mapAttrsFlatten = f: r: map (attr: f attr r.${attr}) (attrNames r);

mapAttrsToList:

nixpkgs/lib/attrsets.nix

Lines 1093 to 1096 in b281753

mapAttrsToList =
f:
attrs:
map (name: f name attrs.${name}) (attrNames attrs);

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: lib The Nixpkgs function library labels Jul 19, 2024
@tie tie changed the title lib/deprecated: suggest using mapAttrsToList instead of mapAttrsFlatten lib/deprecated: alias mapAttrsFlatten to mapAttrsToList Jul 19, 2024
@tie tie requested a review from philiptaron July 19, 2024 08:44
@tie tie marked this pull request as ready for review July 19, 2024 08:44
@tie tie requested a review from infinisil as a code owner July 19, 2024 08:44
lib/deprecated/misc.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 19, 2024
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

This looks right to me, given https://noogle.dev/f/lib/mapAttrsToList. I take no position on the warnIf.

@@ -223,7 +223,7 @@ in

config = mkIf (cfg.servers != { }) {

systemd.services = (listToAttrs (mapAttrsFlatten (name: value: nameValuePair "openvpn-${name}" (makeOpenVPNJob value name)) cfg.servers))
systemd.services = (listToAttrs (mapAttrsToList (name: value: nameValuePair "openvpn-${name}" (makeOpenVPNJob value name)) cfg.servers))
Copy link
Contributor

Choose a reason for hiding this comment

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

Pulled in because of a top-level with statement 😿

Copy link
Member Author

Choose a reason for hiding this comment

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

Welp 🤷
See also #208242

Copy link
Contributor

Choose a reason for hiding this comment

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

See also who it's assigned to. 😉

@tie tie added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 20, 2024
@tie
Copy link
Member Author

tie commented Jul 24, 2024

@philiptaron, are we waiting for a review from @infinisil (ping :]) before merging?

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.

I think a lib.warn would be best! (We'd only need a lib.warnIf (isInOldestRelease 2411) if the replacement were introduced in the same change)

@tie
Copy link
Member Author

tie commented Jul 24, 2024

@infinisil, done. I’ve used isInOldestRelease 2405 since we probably want to start printing warnings for NixOS 24.11 release (and unstable release channel) and hopefully remove this function in future releases to avoid confusion.

@infinisil
Copy link
Member

Ah no we really just need an unconditional lib.warn here. The isInOldestRelease thing is a bit confusing, but it's not needed here, see also #291939

@infinisil
Copy link
Member

Specifically isInOldestRelease is asking "is the new feature that replaces the old one in the oldest supported release?", which is always true if the replacement already exists in all supported releases

@tie
Copy link
Member Author

tie commented Jul 24, 2024

Ah, sorry, misunderstood your suggestion.

@tie
Copy link
Member Author

tie commented Jul 24, 2024

@infinisil, changed to warn about deprecation instead of warnIf.

@infinisil infinisil merged commit 473e469 into NixOS:master Jul 24, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants