docs: experimental re-write #2884
Conversation
|
I completly agree that there is too much nesting! |
MattSturgeon
force-pushed
the
docs/modular
branch
9 times, most recently
from
7d976d6 to
d3f4c0a
Compare
|
I think this is ready for review. Not finished necessarily, but I don't think that's important as this is intended to be marked "beta" or "experimental" for now. |
|
I'd suggest moving the |
|
CI failure is due to the current docs impl not supporting This is a consequence of us hand-rolling evaluating the option tree, instead of leveraging the nixpkgs tooling (i.e. Assuming the new docs & old docs will live alongside each other for a while, I'll fix the old impl in another PR before merging this one. |
MattSturgeon
force-pushed
the
docs/modular
branch
6 times, most recently
from
003271d to
964d98a
Compare
|
I've addressed the feedback so far, and done some additional cleanup. |
There was a problem hiding this comment.
I'm a bit confused why we have:
- The docs.{files,platform,options} options
- Alongside the docs.*.menu.section
I'd assume for example that docs.platforms.menu.section must always be "platforms" or that docs.options.menu.section should be "options"
Also I would find it better if the "docs" documentation was in "Contributors" or "Developpement" section instead of the main "Options" section, as it would not be very relevant to most users
If you could provide a high level overview of the docs module architecture somewhere it could be quite helpful for the review, I still have not built the complete picture in my head (due to going at it only bit by bit)
lib/plugins/utils.nix
Outdated
| docs.options.${docsfile} = { | ||
| title = lib.last loc; | ||
| description = lib.concatLines ( | ||
| [ | ||
| "**URL:** [${url}](${url})" | ||
| "" | ||
| ] | ||
| ++ lib.optionals (maintainers != [ ]) [ | ||
| "**Maintainers:** ${maintainersString}" | ||
| "" | ||
| ] | ||
| ++ lib.optionals (description != null && description != "") [ | ||
| "---" | ||
| "" | ||
| description | ||
| ] | ||
| ); | ||
| }; |
There was a problem hiding this comment.
As a gut reaction it feels a bit strange to have a function called mkMetaModule that sets other things than meta arguments.
It feels a "duplicate" of the metadata in another form, can't we extract it with a well placed library function? (Not entirely sure, as the metadata is a bit of a complicated beast --')
There was a problem hiding this comment.
There is a lot of deliberately duplicated stuff, with the idea being that the "old" way will eventually be removed.
| options.docs._utils = lib.mkOption { | ||
| type = with lib.types; lazyAttrsOf raw; | ||
| description = "internal utils, modules, functions, etc"; | ||
| default = { }; | ||
| internal = true; | ||
| visible = false; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Why do we go with a "utils" module instead of additions to the nixvim lib or to a separate "docs" library? (I'm not suggesting it's better, I genuinely don't know why we should choose one or the other)
There was a problem hiding this comment.
My thought process was that I wanted to avoid bloating our lib with docs-specific things.
I also wanted things in this util to be able to depend on options and config if needed.
Not saying this is the best approach, though.
There was a problem hiding this comment.
I might re-consider this depending on what we end up using _utils for. If it only has a couple things, such as mkOptionList it may be better to have it in lib.nixvim.docs.
The various While we could potentially have a single submodule with a bunch of specialised nullable options, it felt cleaner to keep it separate. I could be persuaded either way, though.
Some page options set a sane default for
This can be done by setting
Sorry, I'll try and get to that. I know I've been a bit lazy documenting and explaining my design & implementation here! |
Don't be sorry, I always find your PR messages very useful & much better than the ones I write |
MattSturgeon
force-pushed
the
docs/modular
branch
from
1e11f8f to
fc972cb
Compare
MattSturgeon
force-pushed
the
docs/modular
branch
2 times, most recently
from
0499f81 to
b022cc4
Compare
| ]; | ||
|
|
||
| settings = writeTOML "book.toml" settings; | ||
| menu = builtins.toFile "menu.md" (builtins.unsafeDiscardStringContext menu); |
There was a problem hiding this comment.
A comment would be nice to say what is discarded, I guess the why is that docs are "inert"?
There was a problem hiding this comment.
I copied this from the nixos docs impl, and there was no comment to copy. I'll try come up with something.
However, yeah "string context" is how nix keeps track of a string's dependencies. E.g. if you do "${pkgs.hello}/bin/hello" then that string has some "context" so that nix knows it has a dependency on the pkgs.hello derivation.
Using unsafeDiscardStringContext drops that context, so unsafeDiscardStringContext "${pkgs.hello}/bin/hello" would have the same string value, but without the dependency on building the pkgs.hello derivation.
In the context of the docs, I believe this is mainly relevant for the "declared by" links, and maybe any default or example values that accidentally used a derivation in its value. I.e. we want the string value, but we don't want the dependency.
| options.docs.menu.src = lib.mkOption { | ||
| type = lib.types.lines; | ||
| description = '' | ||
| MDBook SUMMARY menu. Generated from `docs.pages.<name>.menu`. | ||
|
|
||
| See MDBook's [SUMMARY.md](https://rust-lang.github.io/mdBook/format/summary.html) docs. | ||
| ''; | ||
| readOnly = true; | ||
| }; |
There was a problem hiding this comment.
Maybe add mdbook to the option name as it's specific to it. I'd imagine we'll use the same menu options in some way in man pages
Or maybe the entire menu option is mdbook specific?
There was a problem hiding this comment.
I'm unsure. Currently I think menu as a whole is specific to mdbook's SUMMARY, but I'm unsure. Maybe we'd also have a man-page summary/toc?
Either way, I think menu.src is a bad name. So I'm open to ideas.
There was a problem hiding this comment.
Yeah the entire menu impl is quite mdbook specific.
I don't think man pages tend to have TOCs
| nesting = lib.mkOption { | ||
| type = lib.types.bool; | ||
| description = "Whether pages in this section can be nested within other menu items."; | ||
| default = true; | ||
| }; |
There was a problem hiding this comment.
Isn't this a bit misleading, as it controls both nesting & numbering/un-numbering of sections
In addition from what I understand you can't mix both types arbitrarily:
- Prefix Chapter
- Numbered Chapter
- Suffix Chapter
There was a problem hiding this comment.
It's more of an internal option, describing how the section should be rendered. I.e. whether it is a prefix/suffix section or not.
Only prefix/suffix sections support non-list items, and non-list items cannot be nested.
Maybe there's a better way to represent this, e.g. splitting up menu into three options:
menu.prefixmenu.mainmenu.suffix
Or something else?
There was a problem hiding this comment.
Yeah I'd suggest having those three options
Introduce various `docs.*` options where doc pages, menu entries, etc can be defined. The options themselves are responsible for rendering this to markdown and HTML.
MattSturgeon
force-pushed
the
docs/modular
branch
from
4d3b447 to
63b9177
Compare
I've spent a couple hours putting together a proof-of-concept for a simpler, more modular & cachable, implementation of the docs.
An attempt is made to lean more heavily on upstream tooling (nixos-render-docs), and to minimise the amount of custom option-traversial.
Instead, the idea is that nixvim's modules are evaluated once, and the resulting
optionsset is partitioned up into smaller sub-sets that will each be rendered individually.This allows us to retain a multi-page architecture while still using the upstream options rendering.
However, it is my opinion that the current docs have too much nesting, so in this new design we have only one level of nesting per options sub-set.
Menu
colorschemes
options
plugins
This could be improved on by having a few exceptions to the rule, e.g. lsp servers. Perhaps a
metaoption could be introduced to indicate whether an option-set should be nested further?This is an incomplete proof-of-concept. Some things are missing, such as the per-platform options docs.
Once the basics are in place, I'd like to have this merged so that users and contributors can experiment with using the beta-docs alongside the "stable" docs.
I don't think all the rough edges and design decisions need to be resolved before merging the initial beta. But feedback is of course encouraged!
Testing
You can test by building
beta-docsordocs.passthru.beta-docs.Or by building
docsas normal and navigating to/beta/index.html.Performance
The eval/build is still quite expensive, but it feels faster than the current docs?