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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: lib.asserts migrate to doc-comments #292310

Merged
merged 1 commit into from Mar 7, 2024

Conversation

hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Feb 29, 2024

Description of changes

Migrate deprecated nixdoc comments to official doc-comment format. (see rfc45)

I choose lib.asserts, as this is a relatively small set that allows us to gradually figure out what parts are missing / should work better.

Manual still renders nicely. The only thing i realized is that examples are not counted by nixos render docs anymore. We could change nixos-render docs, to count markdown headings, instead of json fields to fix this.

@DanielSidhion Not sure if this is a blocker, that we need to resolve beforehand.

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.

Copy link
Member

@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.

Thanks for doing a smaller PR first so we can figure out the format and other minor things! I made comments mostly to make the structure follow what we've been doing with functions outside lib and to follow the docs conventions we have established already.

Thank you for your work!

lib/asserts.nix Outdated Show resolved Hide resolved
lib/asserts.nix Outdated Show resolved Hide resolved
lib/asserts.nix Outdated Show resolved Hide resolved
lib/asserts.nix Outdated
Comment on lines 26 to 17
`pred`

: Predicate that needs to succeed, otherwise `msg` is thrown

`msg`

: Message to throw in case `pred` fails
Copy link
Member

Choose a reason for hiding this comment

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

I've been documenting the types next to the arguments/attributes, but the lib functions have the "Type" header which has this information too, although separate from the actual arguments/attributes. I like the "Type" header because it gives a good high-level shape of the function, but I'm suggesting a few improvements on the argument list to keep it closer to the docs for functions outside lib:
(applies to other functions too)

Suggested change
`pred`
: Predicate that needs to succeed, otherwise `msg` is thrown
`msg`
: Message to throw in case `pred` fails
1. `pred` (Bool)
: Predicate that needs to succeed, otherwise `msg` is thrown
2. `msg` (String)
: Message to throw in case `pred` fails

Justification for the suggestions:

  • this function takes two arguments, so the ordered list helps the reader know what argument they're looking at (e.g. it makes it easy for them to look up the 4th argument to a 5-argument function, for example).
  • Let's also keep the type close to the arguments to make it easier for someone looking up a specific argument in isolation.

(In case one of these arguments were an attribute set, I already tested that using nested definition lists works in the rendered manual)

Copy link
Contributor Author

@hsjobeki hsjobeki Mar 4, 2024

Choose a reason for hiding this comment

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

It seems this doesn't work with definition list. It causes the whole thing to be an ordered list and overrides the definition list behavior.

image

We can work around this by escaping the dot with 1\. syntax.

But typing this kinda feels strange especially widely adopting this in all our comments

    # Inputs

    1\. `name`
    
    : The name of the variable the user entered `val` into, for inclusion in the error message
    
    2\. `val`
    
    : The value of what the user provided, to be compared against the values in `xs`
    
    3\. `xs`
    
    : The list of valid values

Copy link
Contributor Author

@hsjobeki hsjobeki Mar 4, 2024

Choose a reason for hiding this comment

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

So here the (escaped) screenshot for completeness

image

Copy link
Member

Choose a reason for hiding this comment

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

Can you try with the suggestion from my first comment? Note that the suggestion includes extra indentation for the lines that start with : ...content. Without that extra indentation, you'll hit the issue you described above.

Copy link
Contributor Author

@hsjobeki hsjobeki Mar 5, 2024

Choose a reason for hiding this comment

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

Ok. I just tried. Works, but only beeing very accurate with exactly 4 spaces of indentation.

Most people will have their tab on 2 spaces which increases the error potential.

I'm afraid that this convention is very brittle and leads to a lot of rendering problems when people put 0-3 spaces in front. (Which is also very hard to see as whitespace is invisible)

The syntax of the definition list is confusing enough for most, adding these special indentation rules or causing rendering issues is not really what I'm aiming for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This comment is not actionable, these are merely long-term considerations.

This stuff has been bugging me for years now. Right now it looks like you keep the comments attached to arguments - it seems that nixdoc will render those as well.

I think the exact syntax for documenting arguments, with types and prose, doesn't really matter, as long as it's distinct and not too hard to get right. But that means there is room to make something up that fits our needs.

A few thoughts on this:

  • The nixdoc pattern does not allow documenting arguments of partially applied functions.
  • There must be a single source of truth. Having both the signature and the argument listing duplicates information. If we had a structured format for argument listings, the signature can be inferred automatically.
  • Ideally we'd have a distinct syntax, but inventing more languages is bad, because that means more code that needs to be maintained.

I briefly discussed this with @hsjobeki, but what if for example we wrapped all library functions in functors in the source, and unwrapped them programmatically at the top level? Then we could do all sorts of things, such as declaring the signature in the Nix language, rendering documentation from it, generating random values for testing based on that, dynamic optional type checking, ... There is additional evaluation overhead to consider, but otherwise - what do you think? CC @infinisil

Copy link
Contributor Author

@hsjobeki hsjobeki Mar 6, 2024

Choose a reason for hiding this comment

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

I think ideally argument documentation must happen by the user inside the doc-comment.
As of RFC145 it is okay to document structured attributes outside of the doc-comment, such as:

/** Doc comment*/
{
  /** Doc for attribute 'foo' */
  foo
}:
 foo

We should come up with a nice convention, i think it could also make sense to have everything at one place; in the doc-comment.

Less magic, keep it simple.

/** 
Doc comment

# Inputs

...some nice convention to document foo?

*/
{
  foo
}:
 foo

Yes this has pros / cons, but one of the big pro is that it reduces technical complexity by a lot.

lib/asserts.nix Show resolved Hide resolved
lib/asserts.nix Outdated Show resolved Hide resolved
@fricklerhandwerk fricklerhandwerk merged commit 00f00e0 into NixOS:master Mar 7, 2024
20 checks passed
@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-07-documentation-team-meeting-notes-112/40963/1

@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-14-documentation-team-meeting-notes-113/41462/1

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.

None yet

4 participants