-
-
Notifications
You must be signed in to change notification settings - Fork 266
Try an auto-generated doc for disko #1122
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughReplaced CLI-focused Disko documentation with an auto-generated NixOS module options reference. Removed narrative/manual CLI sections and added structured, typed option schemas for module configuration, including nested submodules and examples. The document is generated via nix-options-doc and notes generation metadata. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes ✨ Finishing Touches🧪 Generate unit tests
Comment |
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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/reference.md (2)
1-3
: Add an “autogenerated, do not edit” banner and reproducibility metadataMake the file self-describing and reproducible (pin tool rev and source commit). Minimal header:
+# NOTE: This file is autogenerated. Do not edit by hand. +# Generated with: nix build github:Thunderbottom/nix-options-doc && ./result/bin/nix-options-doc --path lib/ --out docs/reference.md +# Source commit: <disko commit SHA>; Generator rev: <nix-options-doc rev> # NixOS Module OptionsIf you share the exact revs used, I’ll fill them in and update the header.
379-389
: Improve “Deprecated” rendering for keyFileThe deprecation note is easy to miss in a dense reference. Consider prefixing with a clear badge or stronger styling (e.g., “DEPRECATED: use passwordFile or settings.keyFile”).
- DEPRECATED use passwordFile or settings.keyFile. Path to the key for encryption + DEPRECATED — use passwordFile or settings.keyFile. Path to the key for encryption.
## [`options.type`](types/lvm_pv.nix#L12) | ||
|
||
Type | ||
|
||
**Type:** `lib.types.enum [ "lvm_pv" ]` | ||
|
||
## [`options.device`](types/lvm_pv.nix#L17) | ||
|
||
Device | ||
|
||
**Type:** `lib.types.str` | ||
|
||
**Default:** `device` |
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.
🛠️ Refactor suggestion
Broken relative links to source files (types/...)
All links like (types/xxx.nix#L…)
are broken from docs/reference.md
because they resolve to docs/types/...
instead of the repo’s lib/types/...
. Use ../lib/types/...
(or repo-absolute permalinks) so the links work on GitHub.
Run this to verify the current breakage and the proposed fix:
#!/usr/bin/env bash
set -euo pipefail
echo "Checking existing links that start with 'types/'..."
broken=0
while read -r p; do
if [[ ! -e "docs/$p" ]]; then
echo "MISSING: docs/$p"
((broken++)) || true
fi
done < <(grep -oP '\]\(\Ktypes/[^)#]+' docs/reference.md | sort -u)
echo "Broken count: $broken"
echo "Preview sed-based rewrite to ../lib/types/ (no in-place change):"
sed -n '1,120p' docs/reference.md | sed $'s#](types/#](../lib/types/#g' >/dev/null
echo "After-rewrite existence check:"
fixed_missing=0
while read -r p; do
target="lib/${p#types/}"
if [[ ! -e "$target" ]]; then
echo "STILL MISSING (verify path): $target"
((fixed_missing++)) || true
fi
done < <(grep -oP '\]\(\Ktypes/[^)#]+' docs/reference.md | sort -u)
echo "Still missing after rewrite: $fixed_missing"
Apply this mechanical change (globally):
-## [`options.type`](types/lvm_pv.nix#L12)
+## [`options.type`](../lib/types/lvm_pv.nix#L12)
I can push a commit rewriting all (types/…)
links to ../lib/types/…
or convert them to repo permalinks if you prefer.
🤖 Prompt for AI Agents
In docs/reference.md around lines 4-16 the markdown links that reference
(types/xxx.nix#L…) are resolving to docs/types/... instead of the repository's
lib/types/ path; update all such links to point to ../lib/types/xxx.nix#L… (or
replace with repo-absolute permalinks) so they resolve correctly on GitHub,
performing a global rewrite of "](types/" → "](../lib/types/" and then verify
each target file exists in lib/types/.
## [`options._parent`](types/lvm_pv.nix#L26) | ||
|
||
**Type:** `any` | ||
|
||
**Default:** `parent` | ||
|
||
## [`options._meta`](types/lvm_pv.nix#L30) | ||
|
||
Metadata | ||
|
||
**Type:** `lib.types.functionTo diskoLib.jsonType` | ||
|
||
**Default:** | ||
|
||
```nix | ||
dev: { | ||
deviceDependencies.lvm_vg.${config.vg} = [ dev ]; | ||
} | ||
``` | ||
|
||
## [`options._config`](types/lvm_pv.nix#L56) | ||
|
||
NixOS configuration | ||
|
||
**Type:** `any` | ||
|
||
**Default:** `[ ]` | ||
|
||
## [`options._pkgs`](types/lvm_pv.nix#L62) | ||
|
||
Packages | ||
|
||
**Type:** `lib.types.functionTo (lib.types.listOf lib.types.package)` | ||
|
||
**Default:** | ||
|
||
```nix | ||
pkgs: [ | ||
pkgs.gnugrep | ||
pkgs.lvm2 | ||
] | ||
``` | ||
|
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.
🛠️ Refactor suggestion
Hide internal implementation options from public docs
Options like _parent
, _meta
, _config
, _pkgs
are implementation details and will confuse users. They also risk becoming accidental API. The generator should exclude:
- any option with
internal = true
- any option whose name starts with “_”
- optionally, anything under a curated “private” predicate
Provide one of:
- Teach nix-options-doc to honor
internal = true
and a “name startsWith '_'” filter - Annotate these in the modules with
internal = true
(and ensure the generator respects it)
I can open a follow-up PR to the generator to support an --exclude-internal
flag and a pattern filter for names.
🤖 Prompt for AI Agents
In docs/reference.md around lines 24 to 66 the generated docs expose internal
implementation options (_parent, _meta, _config, _pkgs) that should be hidden;
fix by either updating the docs generator (nix-options-doc) to skip any option
with internal = true and any option whose name starts with "_" (and add an
optional --exclude-internal flag/pattern filter), or annotate these module
option definitions with internal = true (and ensure the current generator
respects that flag); implement one of these two fixes so internal/private
options are excluded from the public documentation.
_index = lib.mkOption { | ||
type = lib.types.int; | ||
internal = true; | ||
default = diskoLib.indexOf (x: x.name == partition.config.name) sortedPartitions 0; | ||
defaultText = null; | ||
}; | ||
}; |
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.
🛠️ Refactor suggestion
Internal option _index
is rendered despite internal = true
This confirms the generator isn’t honoring internal = true
. Hiding internal options should be treated as a blocker for publishing the reference.
🤖 Prompt for AI Agents
In docs/reference.md around lines 327–333 the internal option `_index` is being
rendered even though it has `internal = true`; update the documentation
generator to skip any option objects whose `internal` property is truthy.
Specifically, find the options rendering loop in the generator, add a guard that
continues/omits rendering when `option.internal === true` (or when truthy),
ensure any indexing or defaults that depend on option order are computed from
the filtered list, and add a test to assert internal options are not emitted in
the generated reference.
Seems like this didn't understand the different types and merged all the options for each of the different types together |
@EliRibble something that could seperate the different types might be a good start and later soemthing that gives an overview what types can be embedded in what types (maybe some sort of diagram)? |
Yeah, I think I can do something like that, though I'm sure I'll have to generate it myself rather than rely on an automated system. How do y'all feel about potentially changing the way that comments are done in the code to work better with auto-generated documentation? |
Ideally if you could make it work without relying on comments that would be great |
This really shouldn't be considered a "real" PR. I noticed that the auto-generated reference documentation has been considered broken since commit ea3e29fe7d02e2d951c9deea93057330d0e18fec, which essentially means it's never worked.
I generated this documentation with:
I don't expect this to be accepted, but wanted to use it to start a discussion - at a high level, what do you want the auto-generated reference documentation to look like? Are you willing to add comments to the disko code in order to improve auto-generated documentation?
Summary by CodeRabbit