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

doc: lib.composeExtensions reference to overlays #325479

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Jul 8, 2024

Description of changes

  • Clarify type signature. packageSet is not defined, strictly speaking overlays can be used to extend attribute sets.
  • Overlay is a common term that is now linked into its concept chapter.
  • Adds concrete function inputs and document them.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Have a minor suggestions, and a big thing, which is that extends is very relevant for the full picture, so I'm proposing to use it in an example instead.

There's probably a few more improvements that can be made now that we have a complete example, but PR suggestions are not great for doing multiple iterations, so I'll have to give the turn to you ;)

lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
@@ -341,13 +353,34 @@ rec {
in fApplied // g final prev';
Copy link
Member

Choose a reason for hiding this comment

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

prev' could be gPrev, or it could be inlined, because the name doesn't really add anything, except an unnecessary pointer in the heap.

lib/fixed-points.nix Outdated Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Jul 8, 2024

Btw much respect for opening this.
I aimed to keep my feedback minimal, but the documentation needed significant improvements also prior to your change and I couldn't find a suitable 'incremental' stopping point.
I hope you don't mind, and on the flip side, it will be much better.

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Jul 9, 2024

Yeah I also realized that understanding overlays is pretty much impossible on their own without also understanding fix ( and later extends).

I just had this idea of a learning path when explaining the concept:
(Initially it is not necessary to understand extends, which makes it easier to understand)

Imagine to use overlays like this. (Which is much simpler to understand before bringing in extends)

fix (flip extensions { /*Initial Prev Set*/ } )

If your initial doesn't depend on final:

Otherwise we could write

fix (final: extensions final { /*Initial Prev Set*/ } )

Which is equivalent to

lib.fix ( lib.extends extensions original );
where
original = final: { /*Initial*/ }

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Aug 9, 2024

@roberth I've split this now into two PRs. This is the other one. #333389

I feel like we are documenting the same thing twice. So i tried to clean up the api, and only document one of the two functions properly. With a reference on the other one.

@hsjobeki hsjobeki force-pushed the doc/lib-overlays branch 2 times, most recently from f8a8a20 to 6e9e26e Compare August 9, 2024 12:51
@hsjobeki hsjobeki requested a review from a team August 9, 2024 12:52
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I hope I didn't go in a circle too much.
My main input is that by referring to overlays instead of writing out all the parameters we can simplify the documentation and make it easier to understand because we don't have to go into a level of detail that doesn't add any new information that wasn't already in the documentation for the overlay signature.
Adding such redundant information could be useful for explanation-type documentation for studying, but this is reference documentation, so it's not the right place for that kind of elaboration.

lib/fixed-points.nix Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
@hsjobeki
Copy link
Contributor Author

All comments should be addressed by now.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

A few important suggestions to make it more coherent semantically.
Other than those, LGTM. Nice improvement ❤️

lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/fixed-points.nix Outdated Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Sep 13, 2024

Thanks!

@roberth roberth merged commit b4292c4 into NixOS:master Sep 13, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants