fix(c++): Formalize the requirement of section title comments in class declaration ordering.#53
fix(c++): Formalize the requirement of section title comments in class declaration ordering.#53davidlion wants to merge 6 commits intoy-scope:mainfrom
Conversation
…s declaration ordering.
|
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:
WalkthroughRestructures the C++ contribution guide's class member declaration order: replaces the previous single-block ordering with explicit per-access-specifier sections titled and ordered as Types; Static constants; Factory methods; Static methods; Static data members; Constructors; Operators; Destructor; Methods implementing ; Methods overriding ; Methods; Data members. Adds a prefatory clarification and reference. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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: 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 `@docs/dev-guide/contrib-guides-cpp.md`:
- Line 135: The reference link for "Google's style guide" uses a malformed
target name (google-cpp-style-declaration-order) causing the reference added
later to be treated as unused; update the inline link to use proper
reference-link syntax by replacing the current malformed target with a valid
reference-style link that matches the reference identifier used at the bottom of
the document (ensure the link text "Google's style guide" points to
[google-cpp-style-declaration-order] so the later reference is recognized).
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 `@docs/dev-guide/contrib-guides-cpp.md`:
- Line 137: Edit the sentence "Under every existing access specifier of a class
we require that declarations are organized into" by inserting a comma after the
word "class" so it reads "Under every existing access specifier of a class, we
require that declarations are organized into"; update the doc line containing
that exact phrase in contrib-guides-cpp.md.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/dev-guide/contrib-guides-cpp.md (1)
137-137:⚠️ Potential issue | 🟡 MinorAdd comma after "class" for improved readability.
The introductory phrase should be separated from the main clause with a comma.
📝 Proposed fix
-Under every existing access specifier of a class we require that declarations are organized into +Under every existing access specifier of a class, we require that declarations are organized into🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dev-guide/contrib-guides-cpp.md` at line 137, In the sentence that begins "Under every existing access specifier of a class we require that declarations are organized into" add a comma after the word "class" so it reads "Under every existing access specifier of a class, we require that declarations are organized into" to separate the introductory phrase from the main clause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev-guide/contrib-guides-cpp.md`:
- Around line 153-156: Update the two headings/descriptions "Methods
implementing InheritedClass" and "Methods overriding InheritedClass" to make
clear that "InheritedClass" is a placeholder: add a short parenthetical note or
sentence (near the headings or immediately after) instructing readers to replace
InheritedClass with the actual base/parent class name used in their code, and
optionally show a tiny example like "e.g., InheritedClass -> BaseWidget"; update
the text surrounding the symbols "Methods implementing InheritedClass" and
"Methods overriding InheritedClass" to include this clarification.
---
Duplicate comments:
In `@docs/dev-guide/contrib-guides-cpp.md`:
- Line 137: In the sentence that begins "Under every existing access specifier
of a class we require that declarations are organized into" add a comma after
the word "class" so it reads "Under every existing access specifier of a class,
we require that declarations are organized into" to separate the introductory
phrase from the main clause.
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 `@docs/dev-guide/contrib-guides-cpp.md`:
- Around line 153-161: Replace raw angle-bracket placeholder occurrences of
<InheritedClass> with an escaped or code-formatted form to prevent Markdown from
treating it as HTML: update the list items "Methods implementing
<InheritedClass>" and "Methods overriding <InheritedClass>" and the final note
so they use backticks (`<InheritedClass>`) or HTML escapes
(<InheritedClass>) instead of raw brackets; locate the strings "Methods
implementing <InheritedClass>", "Methods overriding <InheritedClass>", and the
line "Note, <InheritedClass> is a placeholder..." and change each instance
accordingly.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95395440-3883-4232-a9a9-fda19b68b040
📒 Files selected for processing (1)
docs/dev-guide/contrib-guides-cpp.md
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 `@docs/dev-guide/contrib-guides-cpp.md`:
- Around line 142-158: The ordered list has inconsistent nesting causing
markdownlint MD029; locate the list items "Types", "Factory methods", "Methods
implementing `InheritedClass`", and "Methods overriding `InheritedClass`" and
fix their nested bullets so the list continuity is preserved: either flatten the
nested lines into the same top-level list or convert the child lines into
properly indented sub-lists using a consistent 4-space indent (or consistent
two-space style used elsewhere) and a correct list marker, ensuring each
sub-item (e.g., "`typedef`, `using`, `enum`", "Declarations of static methods
that return an object of the class being defined.", "Declarations of methods
implementing abstract methods from the class `InheritedClass`.", "Declarations
of methods overriding virtual methods from the class `InheritedClass`.") is
formatted as a valid sub-list entry to avoid breaking the ordered-list sequence.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 869ab365-6ddd-4114-9569-1bde1f1338ac
📒 Files selected for processing (1)
docs/dev-guide/contrib-guides-cpp.md
| 1. Types | ||
| * This includes type aliases (`typedef`, `using`, `enum`), nested structs and classes, and | ||
| `friend` types. | ||
| 2. Static constants | ||
| 3. Factory methods | ||
| * Declarations of static methods that return an object of the class being defined. | ||
| 4. Static methods | ||
| 5. Static data members | ||
| 6. Constructors | ||
| 7. Operators | ||
| 8. Destructor | ||
| 9. Methods implementing `InheritedClass` | ||
| * Declarations of methods implementing abstract methods from the class `InheritedClass`. | ||
| 10. Methods overriding `InheritedClass` | ||
| * Declarations of methods overriding virtual methods from the class `InheritedClass`. | ||
| 11. Methods | ||
| 12. Data members |
There was a problem hiding this comment.
Fix ordered-list nesting to avoid markdownlint MD029 noise.
Line 143, Line 147, Line 154, and Line 156 use nested bullets with indentation that can break ordered-list continuity, which likely causes the MD029 warnings reported for Line 145-Line 158.
Proposed fix
1. Types
- * This includes type aliases (`typedef`, `using`, `enum`), nested structs and classes, and
- `friend` types.
+ * This includes type aliases (`typedef`, `using`, `enum`), nested structs and classes, and
+ `friend` types.
2. Static constants
3. Factory methods
- * Declarations of static methods that return an object of the class being defined.
+ * Declarations of static methods that return an object of the class being defined.
4. Static methods
5. Static data members
6. Constructors
7. Operators
8. Destructor
9. Methods implementing `InheritedClass`
- * Declarations of methods implementing abstract methods from the class `InheritedClass`.
+ * Declarations of methods implementing abstract methods from the class `InheritedClass`.
10. Methods overriding `InheritedClass`
- * Declarations of methods overriding virtual methods from the class `InheritedClass`.
+ * Declarations of methods overriding virtual methods from the class `InheritedClass`.
11. Methods
12. Data members🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 145-145: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/2/3
(MD029, ol-prefix)
[warning] 146-146: Ordered list item prefix
Expected: 2; Actual: 3; Style: 1/2/3
(MD029, ol-prefix)
[warning] 148-148: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/2/3
(MD029, ol-prefix)
[warning] 149-149: Ordered list item prefix
Expected: 2; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 150-150: Ordered list item prefix
Expected: 3; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
[warning] 151-151: Ordered list item prefix
Expected: 4; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 152-152: Ordered list item prefix
Expected: 5; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 153-153: Ordered list item prefix
Expected: 6; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 155-155: Ordered list item prefix
Expected: 1; Actual: 10; Style: 1/1/1
(MD029, ol-prefix)
[warning] 157-157: Ordered list item prefix
Expected: 1; Actual: 11; Style: 1/2/3
(MD029, ol-prefix)
[warning] 158-158: Ordered list item prefix
Expected: 2; Actual: 12; Style: 1/2/3
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/dev-guide/contrib-guides-cpp.md` around lines 142 - 158, The ordered
list has inconsistent nesting causing markdownlint MD029; locate the list items
"Types", "Factory methods", "Methods implementing `InheritedClass`", and
"Methods overriding `InheritedClass`" and fix their nested bullets so the list
continuity is preserved: either flatten the nested lines into the same top-level
list or convert the child lines into properly indented sub-lists using a
consistent 4-space indent (or consistent two-space style used elsewhere) and a
correct list marker, ensuring each sub-item (e.g., "`typedef`, `using`, `enum`",
"Declarations of static methods that return an object of the class being
defined.", "Declarations of methods implementing abstract methods from the class
`InheritedClass`.", "Declarations of methods overriding virtual methods from the
class `InheritedClass`.") is formatted as a valid sub-list entry to avoid
breaking the ordered-list sequence.
2 participants
Description
As title says.
Checklist
breaking change.
Validation performed
CI passes. Declaration ordering and comments applied to some classes in CLP.
Summary by CodeRabbit