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/doc-support: Generalize library docs code #305847

Closed
wants to merge 6 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Apr 21, 2024

Description of changes

This refactors doc/doc-support/lib-function-docs.nix so that it will be usable for other libraries than lib.
Future usages may include:

  • pkgs.formats (currently nixos/doc/manual/development/settings-options.section.md)
  • the nixos/lib/default.nix library

This PR also anticipates

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.05 Release Notes (or backporting 23.05 and 23.11 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.

It was quite small, so the indirection was just going to be annoying.
if [[ -e "../lib/$baseName.nix" ]]; then
nixdoc -c "$name" -d "lib.$name: $description" -l ${locationsJSON} -f "$baseName.nix" > "$out/$name.md"
nixdoc -c "$name" -d "$PREFIXDOT$name: $description" -l ${locationsJSON} -f "$baseName.nix" --prefix "$PREFIX" > "$out/$name.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

This would require nixdoc to support that feature as you already mentioned. I could take care of that in the next couple of days.

library = pkgs.lib;
src = ../lib;
name = "nixpkgs-lib-docs";
prefix = "lib";
Copy link
Contributor

Choose a reason for hiding this comment

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

A am about to change this default in nixdoc. Can you change it to "lib." already here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

So more like a string prefix than a path prefix? That's also fine.

Copy link
Contributor

@hsjobeki hsjobeki Apr 25, 2024

Choose a reason for hiding this comment

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

Isn't that what you requested in nix-community/nixdoc#119? Or did I understand it wong?

This now depends on which one gets merged earlier. Ideally, we would merge this PR first nix-community/nixdoc#122 to support empty prefixes with the default prefix="lib.".

The problem is, that if we change the prefix delimiter in the wrong way. The manual will not build because the heading IDs on the manual change.

If we merge this one first, we are in a deadlock. Where we must implement a backwards compatible migration in nixdoc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both solutions are valid, and I don't have a preference.
Requiring the . to be specified on the command line is more of a breaking change, but the implementation is slightly simpler.

@hsjobeki
Copy link
Contributor

@infinisil We could merge this one i think. I'd like to extend the docs by pkgs.* functions. Maybe i can work that into nixdoc, by using the semi-stable nix-c-bindings next. No sure how that works out with rust ;)

@roberth
Copy link
Member Author

roberth commented Jul 29, 2024

It looks like it has been "generalized" with default arguments.
I don't like that, because it confuses the specifics and the tooling, but either way this patch is useless now and needs to be re-done.

That's fine. It was just a small refactor anyway.
I'd still recommend to factor out a generic function without any of lib specifics, but it's probably most effective that you do it, considering your focus on docs.

@roberth roberth closed this Jul 29, 2024
@roberth
Copy link
Member Author

roberth commented Jul 29, 2024

Ugh, closing feels so harsh? It's fine.

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.

2 participants