lib.types: introduce a fileset type#428293
Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
lib/fileset/default.nix
Outdated
There was a problem hiding this comment.
A non-allocating version of this might be preferable for performance.
There was a problem hiding this comment.
Hum, I'm unsure what would be the way to achieve this. I'm thinking of reworking _coerceResult to _coerceGeneric or something that gets the ok and error functions as arguments, and instantiate _coerce where ok = id and error = throw and isFileset where ok = _: true and error = _: false. Is that what you had in mind? Am I missing a cleaner solution?
There was a problem hiding this comment.
Don't think it's worth it tbh, the fileset library is not that performant anyways (it's not a primary goal either)
There was a problem hiding this comment.
The change I describe above is pretty easy to implement, though. So if it makes it a bit better, I'm happy to oblige.
There was a problem hiding this comment.
Not sure then.
fileset library is not that performant anyways
Ok. As an evaluation-time thing, they do need to be evaluated over and over. That could be problem if a project has many files.
Do you think the library could be improved in this regard? Or should we develop primops, like a native fileset type?
Anyway, let's not block on this and keep it simple for now.
|
I think the current CI failure has nothing to do with this PR, but rather is about CI hitting rate limiting from GitHub. |
nixpkgs-ci
bot
added
the
12.approvals: 1
wegank
added
the
2.status: merge conflict
nixpkgs-ci
bot
removed
the
2.status: merge conflict
|
LGTM, but could you squash/amend the commits so that the |
Thank you for taking the time to review all this! I have almost done what you asked, because of one uncertainty: to be clear, I should also merge the commit that adds the fileset type and the commit that add tests for that type? |
|
@roberth ? |
5 participants
During some work on the Fediversity project, I came with the need for a programmatic FileSet type. I used a bit of an ugly workaround, and this PR is here to introduce a clean alternative.
_coerceinternal function to expose a pure variant,_coerceResult.isFileset(using the new_coerceResult) andemptyIMO, what will deserve most attention is:
My choice for
isFileset, namely anything that can be passed tolib.filesetfunctions, not just something with_type == "fileset'. I could have called it “filesetable” or something, but this is how “FileSet” is used in the documentation.My choices for
checkandmerge, namelylib.fileset.isFilesetandlib.fileset.unions. This is the first time I contribute tolib.types, so I am probably not seeing some interactions they might have.Whether we even want that change in nixpkgs or whether we feel it belongs downstream where it is actually used.
Closes #266366
Related to #266356
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.