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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

treewide: improve usage of lib.optional and lib.optionals #304726

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Apr 17, 2024

This PR updates nixpkgs by replacing instances of lib.optional with lib.optionals where appropriate. This change aims to standardize our usage of these functions and make the codebase more intuitive, especially for new contributors.

The lib.optional function generates a list containing the value if the condition is true, and an empty list if false. The lib.optionals function, on the other hand, doesn't wrap values by default. Here is a quick reminder of how both functions behave:

nix-repl> lib.optionals true "foo"
"foo"
nix-repl> lib.optionals false "foo"
[]
nix-repl> lib.optional true "foo"
[ "foo" ]
nix-repl> lib.optional false "foo"
[]
nix-repl> lib.optional true [ "foo" ]
[ [ ... ] ]
nix-repl> lib.optional false [ "foo" ]
[ ]

During code reviews, I have frequently noticed incorrect usage of lib.optional and lib.optionals. By consistently adopting lib.optionals, we can avoid common errors such as unintentionally nested lists when a flat list is expected. This function's explicit need for bracketed output also improves readability and reduces ambiguity, lowering the barrier for new contributors and ensuring alignment with common Nix naming patterns.

Description of changes

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.05 Release Notes (or backporting 23.05 and 23.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.

Add a 馃憤 reaction to pull requests you find important.

@eclairevoyant
Copy link
Member

eclairevoyant commented Apr 17, 2024

This probably merits some discussion; I'm not sure it's intuitive that lib.lists.optionals can take any type for the second arg and returns if the first arg is true, but returns an empty list if the first arg is false. Presumably we should have some type checking on the second arg if we're to use it universally.

@drupol
Copy link
Contributor Author

drupol commented Apr 17, 2024

Thanks for your input! I actually left this PR in draft to avoid pinging respective maintainers for potentially nothing, and also foster discussions.

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

3 participants