Conversation
Reviewer's GuideThis PR extends the design documentation to cover new Class diagram for updated NetsukeManifest and Target structsclassDiagram
class NetsukeManifest {
Vec~Rule~ rules
Vec~Target~ actions
Vec~Target~ targets
Vec~String~ defaults
}
class Target {
String name
Vec~String~ deps
Vec~String~ order_only_deps
String rule
HashMap~String, String~ vars
bool phony
bool always
}
NetsukeManifest "1" -- "*" Rule : contains
NetsukeManifest "1" -- "*" Target : actions
NetsukeManifest "1" -- "*" Target : targets
Class diagram for updated BuildEdge structclassDiagram
class BuildEdge {
Vec~PathBuf~ outputs
Vec~PathBuf~ implicit_outputs
Vec~PathBuf~ inputs
Vec~PathBuf~ implicit_deps
Vec~PathBuf~ order_only_deps
bool phony
bool always
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughExtend the Netsuke build system design and roadmap documentation to introduce two new boolean target attributes, Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)docs/**/*.mdInstructions used from: Sources:
**/*.mdInstructions used from: Sources:
⚙️ CodeRabbit Configuration File 🪛 LanguageTooldocs/netsuke-design.md[uncategorized] ~156-~156: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [uncategorized] ~216-~216: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [uncategorized] ~216-~216: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) [uncategorized] ~219-~219: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [uncategorized] ~219-~219: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) 🔇 Additional comments (4)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/netsuke-design.md(7 hunks)docs/roadmap.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🪛 LanguageTool
docs/roadmap.md
[uncategorized] ~22-~22: Possible missing comma found.
Context: ...t phony and always boolean flags on targets and parse an optional actions lis...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/netsuke-design.md
[uncategorized] ~149-~149: Loose punctuation mark.
Context: ...ore rendering other sections. - phony: When set to true, the target runs whe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~149-~149: Possible missing comma found.
Context: ...true, the target runs when explicitly requested even if a file with the same name exi...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~152-~152: Loose punctuation mark.
Context: ...he default value is false. - always: When set to true, the target runs on ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~152-~152: Possible missing comma found.
Context: ...set to true, the target runs on every invocation regardless of timestamps or dependenc...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~155-~155: Loose punctuation mark.
Context: ...The default value is false. - rules: A list of rule definitions. Each rule i...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...s to a Ninja rule block.2 - targets: The primary list of build targets. Each...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~162-~162: Loose punctuation mark.
Context: ...a Ninja build statement.3 - actions: A secondary list of build targets. Any ...
(UNLIKELY_OPENING_PUNCTUATION)
| - [ ] Support `phony` and `always` boolean flags on targets and parse an | ||
| optional `actions` list that treats each entry as a `phony` target. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Wrap the new bullet to ≤ 80 columns and add the serial comma.
The added bullet exceeds the 80-column soft limit and omits the Oxford comma mandated by the style guide. Re-wrap and punctuate correctly.
- - [ ] Support `phony` and `always` boolean flags on targets and parse an
- optional `actions` list that treats each entry as a `phony` target.
+ - [ ] Support `phony` and `always` boolean flags on targets, and parse an
+ optional `actions` list that treats each entry as a `phony` target.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - [ ] Support `phony` and `always` boolean flags on targets and parse an | |
| optional `actions` list that treats each entry as a `phony` target. | |
| - [ ] Support `phony` and `always` boolean flags on targets, and parse an | |
| optional `actions` list that treats each entry as a `phony` target. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: Possible missing comma found.
Context: ...t phony and always boolean flags on targets and parse an optional actions lis...
(AI_HYDRA_LEO_MISSING_COMMA)
🤖 Prompt for AI Agents
In docs/roadmap.md around lines 22 to 24, the new bullet point exceeds 80
columns and lacks the serial (Oxford) comma. Reformat the bullet text to wrap
lines at or below 80 characters and add the serial comma before the last item in
the list to comply with the style guide.
| pub actions: Vec<Target>, | ||
|
|
||
| pub targets: Vec<Target>, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the default semantics for actions directly in the struct doc-comment.
Readers jumping to the code block should not have to infer behaviour from
earlier prose. Add a short comment beside the new actions field.
- #[serde(default)]
- pub actions: Vec<Target>,
+ /// A second target list; every entry is implicitly `{ phony: true }`.
+ #[serde(default)]
+ pub actions: Vec<Target>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub actions: Vec<Target>, | |
| pub targets: Vec<Target>, | |
| /// A second target list; every entry is implicitly `{ phony: true }`. | |
| #[serde(default)] | |
| pub actions: Vec<Target>, | |
| pub targets: Vec<Target>, |
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 317 to 319, the new `actions` field lacks
an inline comment explaining its default semantics. Add a brief comment directly
beside the `actions` field in the struct to clearly document its default
behavior, so readers can understand it immediately without referring back to
earlier text.
| - `phony`: When set to `true`, the target runs when explicitly requested even if | ||
| a file with the same name exists. The default value is `false`. | ||
|
|
||
| - `always`: When set to `true`, the target runs on every invocation regardless | ||
| of timestamps or dependencies. The default value is `false`. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Re-wrap the phony/always bullets to honour the 80-column rule.
Both bullets run past the prescribed width. Re-flow to two lines apiece.
- - `phony`: When set to `true`, the target runs when explicitly requested even if
- a file with the same name exists. The default value is `false`.
- - `always`: When set to `true`, the target runs on every invocation regardless
- of timestamps or dependencies. The default value is `false`.
+ - `phony`: When `true`, the target runs when explicitly requested even if a
+ file with the same name exists. The default value is `false`.
+ - `always`: When `true`, the target runs on every invocation, regardless of
+ timestamps or dependencies. The default value is `false`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `phony`: When set to `true`, the target runs when explicitly requested even if | |
| a file with the same name exists. The default value is `false`. | |
| - `always`: When set to `true`, the target runs on every invocation regardless | |
| of timestamps or dependencies. The default value is `false`. | |
| - `phony`: When `true`, the target runs when explicitly requested even if a | |
| file with the same name exists. The default value is `false`. | |
| - `always`: When `true`, the target runs on every invocation, regardless of | |
| timestamps or dependencies. The default value is `false`. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~149-~149: Loose punctuation mark.
Context: ...ore rendering other sections. - phony: When set to true, the target runs whe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~149-~149: Possible missing comma found.
Context: ...true, the target runs when explicitly requested even if a file with the same name exi...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~152-~152: Loose punctuation mark.
Context: ...he default value is false. - always: When set to true, the target runs on ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~152-~152: Possible missing comma found.
Context: ...set to true, the target runs on every invocation regardless of timestamps or dependenc...
(AI_HYDRA_LEO_MISSING_COMMA)
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 149 to 154, the bullet points for `phony`
and `always` exceed the 80-column width limit. Reformat each bullet point so
that the text wraps neatly within 80 characters per line, splitting the
descriptions into two lines each while preserving the original meaning and
clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
docs/netsuke-design.md (6)
156-158: Wrapactionsbullet to ≤ 80 columns and drop literal braces.The line still breaches the project’s 80-column rule and repeats the literal struct-style braces that prose should avoid. Re-flow it and phrase the defaults in plain English.
- - `actions`: A secondary list of build targets. Any target placed here is - treated as `{ phony: true, always: false }` by default. + - `actions`: A secondary list of build targets. Each entry is treated as a + phony target that does not run on every invocation by default.
218-223: Re-wrapphony/alwaysbullets to 80 columns.The bullets still exceed the mandated width and replicate the “When set to
true” boiler-plate. Adopt the earlier suggestion verbatim.- - `phony`: When set to `true`, the target runs when explicitly requested even if - a file with the same name exists. The default value is `false`. - - `always`: When set to `true`, the target runs on every invocation regardless - of timestamps or dependencies. The default value is `false`. + - `phony`: When `true`, the target runs even if an output file already exists. + Default: `false`. + - `always`: When `true`, the target runs on every build regardless of + timestamps or dependencies. Default: `false`.
316-318: Add a doc-comment aboveactionsfield.Maintain parity with other documented fields and surface semantics to IDEs.
- #[serde(default)] - pub actions: Vec<Target>, + /// A secondary target list whose entries are implicitly phony. + #[serde(default)] + pub actions: Vec<Target>,
354-359: Documentphonyandalwaysfields.Doc-comments remain missing despite prior feedback.
- #[serde(default)] - pub phony: bool, - - #[serde(default)] - pub always: bool, + /// Run even when a file with the same name already exists. + #[serde(default)] + pub phony: bool, + + /// Run on every invocation, ignoring timestamps. + #[serde(default)] + pub always: bool,
656-660: Update struct-level summary to mentionphonyandalways.The field-level docs are welcome, yet the opening summary above
BuildEdgestill references only inputs/outputs. Append one sentence so autogenerated docs stay accurate.
724-726: Remove incorrect-t runadvice foralwaystargets.
-t runis a Ninja CLI flag, not a build-file directive, and embedding it will break builds. Describe a supported strategy instead:- If `phony` is `true`, use Ninja's built-in `phony` rule. If `always` is - `true`, append the `-t run` invocation so the command executes every time. + If `phony` is `true`, emit a phony build edge with no outputs. + If `always` is `true`, either emit a dummy output that always depends on an + `alwaysdirty` target or declare a phony rule with no outputs so Ninja + re-executes the step on every build.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/netsuke-design.md(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🪛 LanguageTool
docs/netsuke-design.md
[uncategorized] ~156-~156: Loose punctuation mark.
Context: ...a Ninja build statement.3 - actions: A secondary list of build targets. Any ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~218-~218: Loose punctuation mark.
Context: ...ore rendering other sections. - phony: When set to true, the target runs whe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~218-~218: Possible missing comma found.
Context: ...true, the target runs when explicitly requested even if a file with the same name exi...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~221-~221: Loose punctuation mark.
Context: ...he default value is false. - always: When set to true, the target runs on ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~221-~221: Possible missing comma found.
Context: ...set to true, the target runs on every invocation regardless of timestamps or dependenc...
(AI_HYDRA_LEO_MISSING_COMMA)
leynos
deleted the
codex/extend-target-design-with-phony-and-always-flags
branch
1 participant
Summary
actionsblockphonyandalwaysflags in manifestsTesting
make fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_687296d1a28483229250d4c38fd7db10
Summary by Sourcery
Add support for ‘phony’ and ‘always’ target flags with a shorthand ‘actions’ list and update IR generation, Ninja synthesis, and documentation accordingly
New Features:
Enhancements:
Documentation: