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

init: frontmatter support #114

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hsjobeki
Copy link
Collaborator

@hsjobeki hsjobeki commented Mar 20, 2024

Add support for frontmatter

Why frontmatter is needed (sometimes)

Sometimes it is desireable to extend the content with meta information. Frontmatter is the de-facto standard of adding document specific meta tags to markdown. 1 2 3

This would allow us to keep track of references between documents that are closely related to nix code or externalized doc-comments that are no longer tracked.

This PR also includes a detailed design document.

i.e.,

  • Enriching documentation generation.
  • Maintaining References.
  • Metadata inclusion.
  • Allow better Handling of edge cases.

Example

{
/** 
 ---
 import: ./path.md
 ---
*/
foo = x: x;
}

TODO

  • Unit tests
  • Bump version
  • Manual testing with nixpkgs/nixos manuals

doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
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.

Reviewed together in the docs team meeting!

Note that you could also include the arguments from the meeting into the design document.

doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
src/frontmatter.rs Outdated Show resolved Hide resolved
src/frontmatter.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-03-21-documentation-team-meeting-notes-114/41957/1

@hsjobeki hsjobeki force-pushed the feat/custom-directives branch from e10882a to 6217f2e Compare March 22, 2024 23:33
@hsjobeki hsjobeki force-pushed the feat/custom-directives branch from a2e4bde to 47b1aa1 Compare March 22, 2024 23:40
@hsjobeki hsjobeki force-pushed the feat/custom-directives branch from 082435c to 6267d6a Compare March 23, 2024 10:10
@hsjobeki
Copy link
Collaborator Author

@infinisil I think this PR is ready from my side. If you could give it a quick revise, if all issues are addressed.

Just tested it against nixpkgs with some lib functions. works really nice, didn't notice any problems or issues with the other tooling whatsoever.

Note: I switched to another frontmatter parser library because the error handling wasn't expressive enough.

Copy link

@DanielSidhion DanielSidhion left a comment

Choose a reason for hiding this comment

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

Haven't looked at the code. My review is purely on the design document and README.

README.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
doc/frontmatter.md Outdated Show resolved Hide resolved
Copy link

@DanielSidhion DanielSidhion left a comment

Choose a reason for hiding this comment

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

Approving based only on documentation content, I'm leaving the code review to @infinisil since I have 0 experience on nixdoc code.

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Mar 28, 2024

Reviewed in the docs team meeting:

  • We've discussed this quite intensely, because I have strong doubts this is not an anti-pattern in the long run
  • The motivation as stated is a bit weak; the only concrete problems it would solve are:
    • Editing large doc strings has better tooling support in plain markdown files
    • Avoiding large doc strings in the source makes the code slightly easier to navigate
  • On the other hand:
    • It adds a layer of indirection ('invisible behavior' you have to "kinda know it exists"), e.g. what exactly does doc-location mean?
    • You need to navigate and edit multiple files if the pattern is used
    • I say: When the pattern is used, it's proliferated over time, and I fear it will add friction below the threshold of pain. So it won't get reported to actually get mitigation.
  • This is a trade-off, and there's no obviously correct solution
  • Agreement: Postpone merging until we have a clear-cut use case that would be to painful to support without this feature

(PS: @hsjobeki and @DanielSidhion your effort to improve reference documentation overall is highly laudable, so my critique of this particular pattern is not to dismiss your work.)

@infinisil infinisil marked this pull request as draft March 28, 2024 16:01
@hsjobeki
Copy link
Collaborator Author

hsjobeki commented Mar 28, 2024

When the pattern is used, it's proliferated over time, and I fear it will add friction below the threshold of pain.

@fricklerhandwerk @infinisil.

This fear is justified, and I share it. Nevertheless, it would be very beneficial to provide this feature for individual edge cases with very long documentation. This should not be used extensively, and there are only a handful of places where I can imagine this.
For example, the "mkDerivation" function documentation is rather extensive, and you wouldn't want that in the source code.

So nixdoc is just a documentation tool that offers this feature. It is not an official tool or an opinionated tool. But nixpkgs should adhere to usage guidelines. e.g. to use the doc_location feature only for certain functions.

I am still demanding this feature and planning to add more features to "frontmatter" later on, which might be needed in my vision for improved documentation.

But i am okay to let this PR stay open until we really need it. (e.g. when we can generate documentation for mkDerviation)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-03-28-documentation-team-meeting-notes-115/42329/1

@hsjobeki
Copy link
Collaborator Author

@infinisil @fricklerhandwerk
Another place that i think may profit from this feature.

evalModules
https://github.com/NixOS/nixpkgs/blob/18bcb1ef6e5397826e4bfae8ae95f1f88bf59f4f/lib/modules.nix#L85
Documentation file > 100 lines
https://github.com/NixOS/nixpkgs/blob/master/doc/module-system/module-system.chapter.md

Moving documentation into the .nix source files seems not beneficial.
We could add a limit (say 50 lines) to avoid this feature beeing overused.

This feature can help in some places to restructure the content of documentation into pieces that are directly related to source (hence reference) and the other pieces (guide, tutorials, ... )

Therefore i propose to merge this.

@hsjobeki
Copy link
Collaborator Author

hsjobeki commented Dec 31, 2024

There are some edge cases where position tracking in nix is insufficient yet.

I have the following idea that some doc-comments may include the full path of the nix value they document which allows a more decoupled evolution of the doc-comments from the nix position tracking.

---
path = "pkgs.pythonPackage.untrackable"
---

Some content for an edge case, untrackable function

See also: nix-community/noogle#468

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.

5 participants