Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
flake.nix (1)
349-354: Consider addingshortDescriptionfor consistency.Other derivation-based schemas (like
packagesSchemaanddevShellsSchema) extractshortDescriptionfrom package metadata. Adding this field would provide useful context when inventorying HTML documentation packages.Apply this diff to add
shortDescription:children = builtins.mapAttrs (systemType: drv: { forSystems = [ systemType ]; + shortDescription = drv.meta.description or ""; derivation = drv; evalChecks.isDerivation = checkDerivation drv; what = "HTML documentation"; }) systemsForPackage;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flake.nix(2 hunks)
🔇 Additional comments (1)
flake.nix (1)
411-411: LGTM!The schema export is correctly added to the top-level
schemasmapping.
|
It's worth noting that the order of the attributes in Since |
| htmlDocsSchema = { | ||
| version = 1; | ||
| doc = '' | ||
| The `htmlDocs` flake output defines packages providing static HTML output. |
There was a problem hiding this comment.
It would be nice if we had a convention/requirement about what the entry point to the docs should be (e.g. there should be an $out/index.html) that we could document here (and maybe even enforce in the schema, though I don't think the meta-schema currently supports that). That way, tools like FlakeHub could provide a link to the docs automatically.
Currently this doesn't seem to be the case for Nixpkgs (it has entry points like $out/share/doc/nixpkgs/index.html).
There was a problem hiding this comment.
That would indeed be a nice convention. If you think the Nixpkgs folks would be amenable I could propose that upstream, but I suspect that a lot of things probably depend on that directory structure at this point.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Line 376: The identifier checkDerivation is undefined causing the eval
failure; replace the use of checkDerivation in the assignment to
evalChecks.isDerivation with the same pattern used elsewhere: evaluate
drv.attrs.type or null == "derivation" (or define a small helper if you prefer)
so change the line assigning evalChecks.isDerivation (which currently references
checkDerivation and drv) to use drv.attrs.type or null == "derivation" (or add a
local function named checkDerivation that returns attrs.type or null ==
"derivation" and call it with drv).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
flake.nix (2)
373-378: Consider addingshortDescriptionfor tooling consistency.Other derivation-based schemas (e.g.,
derivationsInventoryat line 432,formatterSchemaat line 177) includeshortDescription = drv.meta.description or "";to surface package descriptions innix flake showoutput. This field is missing here.📝 Proposed enhancement
children = builtins.mapAttrs (systemType: drv: { forSystems = [ systemType ]; derivation = drv; + shortDescription = drv.meta.description or ""; isFlakeCheck = false; what = "HTML documentation"; }) systemsForPackage;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 373 - 378, The HTML documentation derivation mapping under children uses builtins.mapAttrs over systemsForPackage but omits shortDescription; add a shortDescription attribute to each mapped object (e.g., inside the mapAttrs body where forSystems, derivation, isFlakeCheck, and what are set) and set it to drv.meta.description or "" so tooling like nix flake show surfaces package descriptions (reference symbols: children, builtins.mapAttrs, systemsForPackage, derivation).
360-381: Add test fixtures for the new schema.The test harness (per
tests/flake.nixlines 72-90) automatically discovers test fixtures by scanning fortests/*.jsonfiles and expects a correspondingtests/<name>/flake.nix. Without these fixtures, thehtmlDocsSchemawon't be validated by CI.Consider adding:
tests/htmlDocs/flake.nix— a minimal flake exposinghtmlDocs.<package>.<system>outputstests/htmlDocs.json— the expected JSON fromnix flake showWould you like me to generate skeleton test fixtures following the pattern in
tests/checks/flake.nixandtests/packages.json?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 360 - 381, Add CI test fixtures for htmlDocsSchema by creating a minimal test fixture named "htmlDocs": add a small flake that exposes outputs in the form htmlDocs.<package>.<system> (so the harness can discover htmlDocsSchema) and add the expected nix flake show JSON for that flake as the accompanying expected-output JSON; follow the existing tests/checks/flake.nix and packages.json patterns when constructing the flake and JSON so the test harness will validate htmlDocsSchema correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@flake.nix`:
- Around line 373-378: The HTML documentation derivation mapping under children
uses builtins.mapAttrs over systemsForPackage but omits shortDescription; add a
shortDescription attribute to each mapped object (e.g., inside the mapAttrs body
where forSystems, derivation, isFlakeCheck, and what are set) and set it to
drv.meta.description or "" so tooling like nix flake show surfaces package
descriptions (reference symbols: children, builtins.mapAttrs, systemsForPackage,
derivation).
- Around line 360-381: Add CI test fixtures for htmlDocsSchema by creating a
minimal test fixture named "htmlDocs": add a small flake that exposes outputs in
the form htmlDocs.<package>.<system> (so the harness can discover
htmlDocsSchema) and add the expected nix flake show JSON for that flake as the
accompanying expected-output JSON; follow the existing tests/checks/flake.nix
and packages.json patterns when constructing the flake and JSON so the test
harness will validate htmlDocsSchema correctly.
2 participants
With this schema in place,
nix flake show nixpkgs --all-systemsdisplays this forhtmlDocs:I'm not sure this will be used outside of Nixpkgs but it would be nice to be able to support all Nixpkgs outputs.
Summary by CodeRabbit