Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] File set combinators #222981
[WIP] File set combinators #222981
Changes from all commits
e056656
6a5b2ff
14e18fe
4576c67
559d3ef
f9e0c13
49baed0
0e939bf
ab93250
c9b8398
698773b
ca9d82f
dad5189
3941ef7
884ace1
d3d3b8b
f0bd959
7257081
633e785
63957e2
f32627a
5c2a1c7
f27d444
145af37
80e6630
0379f40
5b0229c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
It's surprising to me that there are two different versions of each function, and the list versions end in "s". It made me wonder where the multiple unions were coming from, because I'd think about "the union of three directories", rather than that being two different unions in a fold, which is more of an implementation detail.
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.
I adapted this from Haskell's
Data.Set
functions, which also featuresunion
andunions
.We could also read
unions [ a b c ]
asunion a (union b c)
, which then does have multipleunion
s, though that's a bit of a stretch.Do you perhaps have any suggestions for better names?
unions
is probably one of the most useful functions, so I like how it's currently fairly short.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.
Agreed on this, but is
union
(no s) useful? Maybe we just need the list versions, and then we could call themunion
,intersect
, etc? In Nix we're a lot less likely to use them as arguments to higher-order functions likefold
than might be the case in Haskell, where functions that operate on exactly two arguments might be more useful.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.
I imagined that two-argument functions would still be preferred for use cases like "I have a file set, intersect that with only files in ./lib", which would be e.g.
But I'm just realizing that the list-based one isn't much worse:
Though it does require adding a
]
at the end, which isn't the case for the two-argument function.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.
I think it's good to have both ways for convenience, and the
s
for lists is not the worst. If we can come up with a more intuitive naming for non-Haskellers that would be great.