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

feat: Support descriptions from comments #70

Merged
merged 1 commit into from Nov 17, 2023

Conversation

phaer
Copy link
Member

@phaer phaer commented Aug 31, 2023

This extends the existing --description flag with two options:

  • "comment" reads a comment before the first node in the nix input file and sets description to that.
  • "file:$filename" reads a file and sets description to it's verbatim output.

This is the most minimal implementation providing the features I was looking for. Happy to extend it w.r.t. to white-space handling and ideally getting section link anchors back for included headers; which would require us to parse the input.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@asymmetric asymmetric left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test!

I'm pretty happy with the fix for #19, not entirely sure about the file: feature yet, see inline comments.

In any case, I think I'd prefer not using magic strings inside the --description flag (as it would technically be backwards-incompatible, and somewhat surprising), and instead add a couple flags.

Provided we decide to accept both additions, they could be --description-extract and --description-file, so we keep them all close to each other in --help.

src/main.rs Outdated Show resolved Hide resolved
@phaer
Copy link
Member Author

phaer commented Sep 4, 2023

Thanks for the feedback! Just pushed a version with separate flags. Let me know if should remove ´--description-file´

@phaer phaer force-pushed the more-descriptions branch 2 times, most recently from db8692e to 12a717c Compare September 6, 2023 09:15
@phaer phaer changed the title feat: Support descriptions from comments & files feat: Support descriptions from comments Sep 6, 2023
Copy link
Collaborator

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

This does not what I expect it to do. With this PR, running

nixdoc -c test -d test --description-extract -f test.nix

on

# Some file description
{
  /* Some function description */
  f = x: x;
}

gives

Some file description
## `lib.test.f` {#function-library-lib.test.f}

Some function description

`x`

: Function argument

which seems wrong. The text should be under the header.

Edit: Oh I mistook the function header for the section header, so never mind this!


Furthermore, not having any description at all makes it fail:

{
  /* Some function description */
  f = x: x;
}
thread 'main' panicked at 'could not read description from top-level comment', src/main.rs:380:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is in contrast to the current nixdoc which always falls back to something sane in case of missing data. And in this case the default should obviously be an empty description.


Also the flag should be renamed. --description-extract sounds like it would be related to the --description flag, when it doesn't really seem to be.

I'd also be in favor of just not having a flag for this at all though, this seems like sane default behavior.


Regarding use cases, this would also be very useful for Nixpkgs (the primary user of nixdoc), e.g. for the new fileset library I wrote, which currently needs to have its introductory section separately from all the functions: https://nixos.org/manual/nixpkgs/unstable/#sec-fileset

@phaer
Copy link
Member Author

phaer commented Nov 2, 2023

Furthermore, not having any description at all makes it fail:
This is in contrast to the current nixdoc which always falls back to something sane in case of > missing data. And in this case the default should obviously be an empty description.

The behavior you describe happens only if you explicitly pass the flag. Without that flag, there's no change to behavior. I think its preferable to raise an error if the flag is passed and there's no comment, seems less surprising than silently ignoring the flag.

Also the flag should be renamed. --description-extract sounds like it would be related to the --description flag, when it doesn't really seem to be.

Could you elaborate on this? How would you call the extracted text here and what would be the difference to a ?description"?

I'd also be in favor of just not having a flag for this at all though, this seems like sane default behavior.

Happy to do this, but as @asymmetric disliked expanding an existing flag for backwards-compatibility, I assumed changing default behavior would be even less likely to get merged ;)


Happy to continue with this. Another open question we've discussed at NixCon was whether it's deemed worth the effort to parse and re-serialize the descriptions in order to insert anchors to headings. I think it's fair to leave that functionality to the actual renderers, as i.e. Github and mdbook support that by themselves.

@infinisil
Copy link
Collaborator

Happy to do this, but as @asymmetric disliked expanding an existing flag for backwards-compatibility, I assumed changing default behavior would be even less likely to get merged ;)

I don't think this would be a problem for Nixpkgs. If at all, header comments in lib functions either are inexistent or describe the sublibrary, which is what we want. So this would be my preference. Because of nixdoc's state of mainly being used for Nixpkgs and being fairly ad-hoc, I don't think we need to have strong backwards-compatibility guarantees.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-11-09-documentation-team-meeting-notes-93/35244/1

@phaer phaer force-pushed the more-descriptions branch 2 times, most recently from 2fe77d9 to 7e9c303 Compare November 9, 2023 22:50
@phaer
Copy link
Member Author

phaer commented Nov 9, 2023

@infinisil Seeing you became the new maintainer of nixdoc - thanks! 🎉 - I don't think we need additional feedback to make it a default behavior. Let me know what you think! :)

Copy link
Collaborator

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Not sure how long-lived nixdoc will still be with RFC 145, but let's keep it working! Still waiting for @asymmetric to make the transfer though :)

Got some suggestions for now though (also the test output needs to be updated once, for an additional newline)

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Check if the first node of a given nix file is a comment
and if so, insert them after the files heading while correcting
whitespace.
@phaer
Copy link
Member Author

phaer commented Nov 10, 2023

Not sure how long-lived nixdoc will still be with RFC 145, but let's keep it working!

If I read it correctly, RFC 145 doesn't say anything about a tool to render markdown/html documentation and neither do the linked PoCs. Am I missing something here? As stated above my primary motivation is code that lives outside of nixpkgs.

@infinisil
Copy link
Collaborator

Ah that's true. I kind of assumed that the RFC 145 shepherd team was working on their own tooling to replace this one. I created #90 to discuss this.

my primary motivation is code that lives outside of nixpkgs.

Haven't considered this very thoroughly, but I guess that's a valid use case, so let's continue supporting it :)

@infinisil infinisil merged commit 15a5e8d into nix-community:master Nov 17, 2023
2 checks passed
@phaer phaer deleted the more-descriptions branch November 17, 2023 16:49
infinisil added a commit to tweag/nixpkgs that referenced this pull request Nov 19, 2023
Previously the introductory section and the function listings were in
different places. But now nixdoc supports having them together
with nix-community/nixdoc#70!
infinisil added a commit to tweag/nixpkgs that referenced this pull request Nov 19, 2023
Previously the introductory section and the function listings were in
different places. But now nixdoc supports having them together
with nix-community/nixdoc#70!
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Nov 20, 2023
Previously the introductory section and the function listings were in
different places. But now nixdoc supports having them together
with nix-community/nixdoc#70!
0x4d6165 pushed a commit to 0x4d6165/nixpkgs that referenced this pull request Nov 20, 2023
Previously the introductory section and the function listings were in
different places. But now nixdoc supports having them together
with nix-community/nixdoc#70!
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Nov 26, 2023
Previously the introductory section and the function listings were in
different places. But now nixdoc supports having them together
with nix-community/nixdoc#70!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants