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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib.packagesFromDirectoryRecursive: Improved documentation #359898

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Nov 28, 2024

For lib.filesystem.packagesFromDirectoryRecursive:

  • provide a more-precise type signature than AttrSet -> AttrSet ;
  • explain what the callPackage parameter is, provide a more-precise type signature for it as part of the function's ;
  • remove boilerplate from the “Inputs” section to improve readability ;
  • explain each example ;
  • warn of the function's limitation with scopes, i.e. that it is unable to make nested scopes for sub-directories.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested building the nixpkgs manual (nix-build doc/)
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Pulled above the inputs section to avoid duplicating information.
@nbraud nbraud requested review from 9999years, Gabriella439 and a team November 28, 2024 17:05
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Nov 28, 2024
@nix-owners nix-owners bot requested a review from infinisil November 28, 2024 17:06
@nbraud nbraud added the 8.has: documentation This PR adds or changes documentation label Nov 28, 2024
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 29, 2024
directory = ./my-packages;
}
)
=> { ... }
```

Here, if `directory` contains `awoo.nix` and `owo/uwu.nix`, with the latter expecting `{ awoo }` as a parameter,
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

@nbraud nbraud Dec 1, 2024

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.

Copy link
Contributor

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

Copy link
Contributor Author

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)

@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation and removed 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1 labels Dec 1, 2024
@nix-owners nix-owners bot requested a review from GetPsyched December 1, 2024 09:44
@nbraud
Copy link
Contributor Author

nbraud commented Dec 1, 2024

Reworked the wording somewhat, and pulled the warning box out of the example (and into its own commit as well)

This should be good to go once @hsjobeki answers the question about choice of names (and the feedback is addressed)

@nbraud
Copy link
Contributor Author

nbraud commented Dec 1, 2024

Fixed issue with nesting blocks, squashed fixup commits

@nbraud nbraud force-pushed the lib/fs/packagesFromDir branch from 5ffd49e to 5b8197c Compare December 1, 2024 11:35
@github-actions github-actions bot removed the 8.has: documentation This PR adds or changes documentation label Dec 1, 2024
@nbraud nbraud removed the request for review from GetPsyched December 1, 2024 11:54
@nbraud nbraud added the 8.has: documentation This PR adds or changes documentation label Dec 1, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 1, 2024
@github-actions github-actions bot removed the 8.has: documentation This PR adds or changes documentation label Dec 2, 2024
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/filesystem.nix Outdated Show resolved Hide resolved
@nbraud nbraud added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 2, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 2, 2024
`directory`
```
packagesFromDirectoryRecursive :: {
callPackage :: Path -> {} -> a,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does {} mean?

Copy link
Contributor Author

@nbraud nbraud Dec 3, 2024

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 _ {}.

Copy link
Contributor

@9999years 9999years left a 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.)

@nbraud nbraud added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 3, 2024
@nbraud
Copy link
Contributor Author

nbraud commented Dec 3, 2024

The CI failure on lib tests is unrelated (some change in modules.nix)

@nbraud nbraud requested a review from hsjobeki December 3, 2024 10:34
nbraud and others added 2 commits December 3, 2024 11:01
Cut out redundant boilerplate, explain what the `callPackage` parameter is.

Co-authored-by: Valentin Gagarin <[email protected]>
nbraud pushed a commit to nbraud/nixpkgs that referenced this pull request Dec 3, 2024
@nbraud
Copy link
Contributor Author

nbraud commented Dec 3, 2024

looking at the manual now I'm seeing other examples of this style.

Yes, there's a significant documentation refactor going on.

(None on more than one line, but they really should be split up a bit like this.)

IDK about nixdoc comments, but the nixpkgs manual has some types written over multiple lines, such as

runCommandWith :: {
  name :: name;
  stdenv? :: Derivation;
  runLocal? :: Bool;
  derivationArgs? :: { ... };
} -> String -> Derivation

(granted, I knew of that one because I wrote it, but I was pointed at other similar examples when I did 😅 )

@nbraud
Copy link
Contributor Author

nbraud commented Dec 4, 2024

Squashed fixup commits in preparation for merge, no change (so not invalidating reviews, CI will give the same results, etc.)

@nbraud nbraud force-pushed the lib/fs/packagesFromDir branch from b63a867 to 25bdcd5 Compare December 4, 2024 16:37
@nbraud nbraud merged commit 2f9d395 into NixOS:master Dec 4, 2024
17 of 18 checks passed
@nbraud nbraud deleted the lib/fs/packagesFromDir branch December 4, 2024 16:45
@9999years
Copy link
Contributor

Yes, there's a significant documentation refactor going on.

Awesome, this looks a lot better so I'm really glad to see that! Thanks again for writing & submitting this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants