-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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/attrsets: add mapCartesianProduct function #298680
Conversation
3311a94
to
bf24278
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
I don't think I've ever seen this operation be called that? Transposing usually means diagonally mirroring a Matrix.
And then I think |
Oh I got the Though, λ sequence [[1,2,3],[4,5,6]]
[[1,4],[1,5],[1,6],[2,4],[2,5],[2,6],[3,4],[3,5],[3,6]] It is more generic than this, but it results in the Cartesian product of every item when dealing with lists of lists.
I agree. This would mean deprecating the existing
Definitely makes sense, would be happy to adjust to the suggested names, thanks for reviewing! |
bf24278
to
32e4ec9
Compare
I'd say that's half of a coincidence, because it relies on the
Yeah I guess that would be best. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also separate each change into a separate commit, e.g.:
- Renaming
cartesianProductOfSets
- Tree-wide replacement
- Introducing
mapCartesianProduct
and introducing its tests - Tree-wide replacement
- Applying lints
Should've looked at your profile before, we could discuss ZipLists another day 😄
Can you elaborate on what this means? I guess it's running some kind of formatter, but I'm not familiar with the current process. |
@infinisil could you have a look at the remaining open questions when you get a chance? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the commits needing cleanup this looks good to me!
5339c25
to
7b687a5
Compare
@infinisil done ✅ |
LGTM except for the 'apply lints' commit that doesn't seem very related |
It is not related indeed, but I had to make changes on that file and noticed a lot of things that can be removed. I don't think anybody would care to submit a PR to remove unused bindings to be honest, but happy to adjust the commit message if there are better suggestions. |
I think it's ok on second thought, thanks! |
Description of changes
The
cartesianProductOfSets
function was added some time ago (akatranspose
orsequence
in other functional languages), while deprecating thecrossLists
function. I have come across the deprecation message "use cartesianProductOfSets instead" without further details, which motivated me to improve the situation by adding a new convenient function. Example:It's the equivalent of using the former followed by
map
, which seems to be quite a common pattern to justify a single function:Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.