-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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.packagesFromDirectoryRecursive: Improved documentation #359898
Conversation
Pulled above the inputs section to avoid duplicating information.
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.
Nice!
lib/filesystem.nix
Outdated
directory = ./my-packages; | ||
} | ||
) | ||
=> { ... } | ||
``` | ||
|
||
Here, if `directory` contains `awoo.nix` and `owo/uwu.nix`, with the latter expecting `{ awoo }` as a parameter, |
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.
Can you make up better directory and file names. Maybe just copy a simple real world example. I find those names a bit confusing.
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.
The other examples use a
, b
, etc. Would that be better?
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.
PS: In the meantime, I added a rendering of the directory tree, which should help grasp the situation better.
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.
If the other ones you found use 'a' and 'b' i would like you to stick to that. Its more simple and lets the Reader focus on what is important
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.
Done! Please re-review and approve (if there's no other feedback to address)
This should be good to go once @hsjobeki answers the question about choice of names (and the feedback is addressed) |
Fixed issue with nesting blocks, squashed fixup commits |
5ffd49e
to
5b8197c
Compare
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.
LGTM
`directory` | ||
``` | ||
packagesFromDirectoryRecursive :: { | ||
callPackage :: Path -> {} -> a, |
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.
What does {}
mean?
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.
The empty attrset; Path -> {} -> a
is a supertype of Path -> Attrset -> a
, which is (closer to) pkgs.callPackage
's type.
In other words, the type annotation guarantees promises that packagesFromDirectoryRecursive
will only ever invoke callPackage _ {}
.
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.
LGTM. The 'type annotations' are kind of a headache because there's no real grammar (or checker, of course) for them so they're all ad-hoc and inconsistent. The old style is what was standard when I wrote this, but looking at the manual now I'm seeing other examples of this style. (None on more than one line, but they really should be split up a bit like this.)
The CI failure on lib tests is unrelated (some change in |
Cut out redundant boilerplate, explain what the `callPackage` parameter is. Co-authored-by: Valentin Gagarin <[email protected]>
…about scope limitation
Yes, there's a significant documentation refactor going on.
IDK about
(granted, I knew of that one because I wrote it, but I was pointed at other similar examples when I did 😅 ) |
Squashed fixup commits in preparation for merge, no change (so not invalidating reviews, CI will give the same results, etc.) |
b63a867
to
25bdcd5
Compare
Awesome, this looks a lot better so I'm really glad to see that! Thanks again for writing & submitting this |
For
lib.filesystem.packagesFromDirectoryRecursive
:AttrSet -> AttrSet
;callPackage
parameter is, provide a more-precise type signature for it as part of the function's ;Things done
nix-build doc/
)Add a 👍 reaction to pull requests you find important.