feat(dev-docs): Require the measurement unit in names if it is not in the static type.#65
feat(dev-docs): Require the measurement unit in names if it is not in the static type.#65davidlion wants to merge 4 commits into
Conversation
WalkthroughThe contributor guide adds a new Naming subsection that defines measurement-unit naming rules, including suffix-based unit naming when type information does not already express the unit. ChangesNaming documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev-docs/dev-guide/contrib-guides-general.md`:
- Around line 13-15: The C++ example text has awkward wording and a missing
comma; revise the sentence in the contribution guide to read more naturally by
adding the comma after “integer type” and rephrasing the “name the variable with
a `_kb` suffix” part so it clearly describes the naming convention rather than
sounding like a command. Update the example sentence in the docs section that
mentions `_kb` and `_ms` suffixes to improve clarity and flow.
- Around line 9-11: The sentence in the contributor guide is missing the comma
in the conditional clause, so update the wording in the measurement-units
guidance to separate “If the unit is not statically available in the type
information” from “then it must be added to the name.” Make this edit in the
paragraph containing the unit naming rule so the readability is improved without
changing the meaning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 908feeba-d00c-47d3-83e6-983e6966a2a0
📒 Files selected for processing (1)
docs/dev-docs/dev-guide/contrib-guides-general.md
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Some questions before I suggest some rephrasing.
| For example, in C++ when storing a measure of kilobytes inside an integer type, name the variable | ||
| with a `_kb` suffix (e.g. `encoded_size_kb`). Similarly, when measuring milliseconds in an integer |
There was a problem hiding this comment.
Hm, we should use "kibibytes" for correctness, right? In which case, should the suffix be kib? Though that might not be very recognizable, lol.
|
|
||
| For example, in C++ when storing a measure of kilobytes inside an integer type, name the variable | ||
| with a `_kb` suffix (e.g. `encoded_size_kb`). Similarly, when measuring milliseconds in an integer | ||
| add an `_ms` suffix (e.g. `parsing_time_ms`). |
There was a problem hiding this comment.
_ms is actually ambiguous as to whether it's milli or mega. We've been trying to do _millis or _millisecs in newer code.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev-docs/dev-guide/contrib-guides-general.md`:
- Around line 16-18: The example in the contributor guide still uses a somewhat
abbreviated unit suffix, which conflicts with the new clarity guidance. Update
the sample identifier in the paragraph to use a fully spelled-out unit name so
the example itself demonstrates the rule; adjust the wording around the
milliseconds example and the identifier shown in the guidance to keep the
convention consistent and self-explanatory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2b22e36-05ef-4aa9-b89a-eaf421479181
📒 Files selected for processing (1)
docs/dev-docs/dev-guide/contrib-guides-general.md
| It is not a requirement to use abbreviations, as the goal is clarity. For example, when measuring | ||
| milliseconds adding `_ms` is actually ambiguous as to whether it's milli or mega, so we use | ||
| `_millisecs` (e.g. `parsing_time_millisecs`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer a clearer unit suffix in the example.
This example still relies on a fairly terse abbreviation, which weakens the “clarity” rule you just introduced. Consider spelling out the unit in the sample identifier so the guidance is self-explanatory.
Proposed wording
-For example, when measuring milliseconds adding `_ms` is actually ambiguous as to whether it's milli or mega, so we use `_millisecs` (e.g. `parsing_time_millisecs`).
+For example, when measuring milliseconds, use a `_milliseconds` suffix (e.g. `parsing_time_milliseconds`).📝 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.
| It is not a requirement to use abbreviations, as the goal is clarity. For example, when measuring | |
| milliseconds adding `_ms` is actually ambiguous as to whether it's milli or mega, so we use | |
| `_millisecs` (e.g. `parsing_time_millisecs`). | |
| It is not a requirement to use abbreviations, as the goal is clarity. For example, when measuring | |
| milliseconds, use a `_milliseconds` suffix (e.g. `parsing_time_milliseconds`). |
🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Consider shortening this phrase to just “whether”, or rephrase the sentence to avoid “as to”.
Context: ...onds adding _ms is actually ambiguous as to whether it's milli or mega, so we use `_millise...
(WHETHER)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dev-docs/dev-guide/contrib-guides-general.md` around lines 16 - 18, The
example in the contributor guide still uses a somewhat abbreviated unit suffix,
which conflicts with the new clarity guidance. Update the sample identifier in
the paragraph to use a fully spelled-out unit name so the example itself
demonstrates the rule; adjust the wording around the milliseconds example and
the identifier shown in the guidance to keep the convention consistent and
self-explanatory.
There was a problem hiding this comment.
@kirkrodrigues I'm guessing there is a better word for "clarity" here that also address the rabbit's concern (I don't fully agree with it, but I see its point).
There was a problem hiding this comment.
@davidlion, since your main concern was ambiguity between "milli" and "mega" rather than clarity in general, you could reframe the goal around unambiguity rather than (or in addition to) clarity. A few options:
- "It is not a requirement to use abbreviations, as the goal is to avoid ambiguity."
- "It is not a requirement to use abbreviations, as the goal is clarity and unambiguity."
- "Abbreviations are not required; prefer whichever form is unambiguous and easy to recognize."
Option 1 directly ties the rule to the actual problem being illustrated (the _ms milli/mega confusion), which should satisfy the original nit without necessarily forcing a fully spelled-out suffix like _milliseconds — you could still keep _millisecs as the example if you feel that's unambiguous enough in your codebase's context. Let me know which direction you'd like and I can help refine the wording further.
2 participants
Description
Added a general section to explicitly name measurement units if not handled by the type.
Checklist
breaking change.
Validation performed
CI passing.
Summary by CodeRabbit
_kbor_millisecs).