-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
complete the markdown transition #40
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-06-26-documentation-team-meeting-notes-58/29666/1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, apart from the last commit, see comments around allow_single_line
test/let.nix
Outdated
*/ | ||
concatStrings = builtins.concatStringsSep ""; | ||
|
||
/* this should be ignored via from */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this is saying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from
is inherited from an explicit scope, so it should not be considered for docs. we'd have to implement an evaluator to do that correctly, so we just ignore it altogether.
) -> Vec<ManualEntry> { | ||
for ev in node.preorder() { | ||
match ev { | ||
WalkEvent::Enter(n) if n.kind() == SyntaxKind::NODE_ATTR_SET => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: document why we're only traversing the body of the let ... in
the prior version with more ad-hoc indenting produced extraneous newlines, breaking deflists in the process. textwrap is a much better solution to indentation, and it composes more easily too.
just dumping the type into the section is only helpful to those who know the haskell type syntax that's often used here, but not always. mark the types with a prominent header to communicate them better.
examples as sections clutters the TOC and removes them from the list of examples. example blocks are supported in nixpkgs markdown, with semantics very similar to old docbook example tags.
lol sorry @pennae I don't know why those two commits landed here, I've reset to your last commit. |
*/ | ||
concatStrings = builtins.concatStringsSep ""; | ||
|
||
/* this should be ignored because it's inherited from an explicit source */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, then I'd change this to:
/* this should be ignored because it's inherited from an explicit source */ | |
/* this should be ignored because it's shadowed by the `inherit ({}) from;` below */ |
this was lost during the markdown transition.
previously we would ignore exported functions that were defined in let bindings. this is obviously not ideal.
line comments on attributes (rather than lambda arguments) are usually used to convey todos or deprecation information. we should probably not include those in the docs.
while the initial commonmark pr was good, it's not enough to fully support nixpkgs. examples were emitted as subsections (dropping them from the table of examples), types of functions weren't marked as clearly as they could've been, function location support was dropped, and the recent rnix update we did even broke parsing in a few places.
this fixes all of that. with this patch set applied we can generate a word-for-word identical nixpkgs manual without involving the docbook toolchain.