chore: code-style fixes per STYLEGUIDE#392
Conversation
- STYLEGUIDE: codify doc-comments as the API source of truth — Aborts lists every error (including propagated), preconditions map to their error, and Parameters/Returns are included only where they add detail beyond the summary; add a pin-exact exception for tolerance/approximation assertions (e.g. fixed-point log/sqrt vs an off-chain reference). - Document struct fields on the access hot-potato structs (delayed/two_step Borrow, two_step RequestBorrow) and fixed_point Components. - Drop the test_ prefix from access_control_tests functions (descriptive names per STYLEGUIDE Testing). Verified with sui move test --lint --warnings-are-errors and build --doc (testnet): access 137, math/core 753, math/fixed_point 426 — all green.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
==========================================
- Coverage 78.90% 78.88% -0.02%
==========================================
Files 23 23
Lines 1782 1781 -1
Branches 640 640
==========================================
- Hits 1406 1405 -1
Misses 341 341
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| #[test] | ||
| #[allow(lint(share_owned))] | ||
| fun test_new_with_otw_succeeds() { |
There was a problem hiding this comment.
Do we have to remove this? Similar to removing test_only from the module definition, I think it may harm readability. I think this prefix helps to quickly recognize that a function is a tests, for example, when you are ctrl+shift+F through the workspace, similar to why keeping the tests suffix in the file name helps with readability even when is not a requirement. cc @0xNeshi for you thoughts too.
There was a problem hiding this comment.
There was a problem hiding this comment.
I agree with Daniel's choice here.
Ctrl+shift+F-ing is straightforward even without the prefix, as you have many indicators you're looking at a test function: #[test] attribute, _tests suffix in the file name, name describes a flow instead of an action (and usually includes succeeds/fails).
I'd go as far as removing the _tests suffix from test module names (like is done in Rust), but having that suffix is a convention in Move on Sui (aligns test module name with the file name).
There was a problem hiding this comment.
This one is the one that has the less strong argument to keep, so I'm ok with removing it. But to me is not the same as removing test_only annotations inside test files, or removing the test suffix on test files, in the sense that there is no an easy visual way then of recognizing these elements without checking the path to see where they are contained, for example when you do ctrl+P on cursor, or when you ctrl+shft+F. It looks like a regression to me on those cases, since there is no clear advantage besides removing a few short lines of code.
There was a problem hiding this comment.
I'm ok with leaving test_ prefix. It doesn't improve clarity for me, but I don't mind leaving it if it helps you.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefines ChangesDocumentation and Test Hygiene
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| - Document public functions with at least `Parameters` and `Returns`; include an | ||
| `Aborts` section whenever a function can abort | ||
| - Document public functions: include `#### Parameters` and `#### Returns` where | ||
| they add detail beyond the one-line summary — a trivial getter whose summary |
There was a problem hiding this comment.
| they add detail beyond the one-line summary — a trivial getter whose summary | |
| they add detail beyond the one-line summary - a trivial getter whose summary |
Update all em dashes into regular dashes, see #388
| already states what it returns needs neither. Always include an `#### Aborts` | ||
| section whenever a function can abort, listing **every** error it can raise — | ||
| including errors propagated from internal calls |
There was a problem hiding this comment.
Apply this rule to all existing functions, i.e. double check if all docs follow this rule.
Also, align the following docs:
- in
math/corenone of thefun inv_modandfun mul_modimplementations list the actual underlying abort error, e.g. see u16.move and u256.move - in
macros.move, in the fun mul_div, the format of the#### Abortsmessage is misaligned with existing convention
| already states what it returns needs neither. Always include an `#### Aborts` | ||
| section whenever a function can abort, listing **every** error it can raise — | ||
| including errors propagated from internal calls |
There was a problem hiding this comment.
Specify should the #### Aborts section appear if no custom errors are emitted.
Two options:
- No section - any native errors (e.g. arithmetic) are implied
- Display section, but only if native errors can appear
Currently there are functions that don't follow any established convention:
- macros.move > mul_shr -> specifies no custom errors are emitted
- macros.move > checked_shl -> specifies that native errors can appear
| section whenever a function can abort, listing **every** error it can raise — | ||
| including errors propagated from internal calls | ||
| - State caller preconditions explicitly and map each to the error it fails with | ||
| (e.g. "Aborts with `EUnauthorized` if the caller lacks the role") |
There was a problem hiding this comment.
| (e.g. "Aborts with `EUnauthorized` if the caller lacks the role") | |
| (e.g. "`EUnauthorized` if the caller lacks the role") |
This is the actual current convention, see access_control.move.
Related to this, align the abort message format of decimal_scaling.move > validate_decimals.
Drop the hardcoded primitive list (access control / math / rate limiting) from the blockquote — it drifts as the library grows. Reframe around discovery.
3 participants
General code-style cleanup against
STYLEGUIDE.md. Does not close any issue.Changes
sui move build --doc, consumed by AI-integrator tooling):#### Abortslists every error a function can raise, including errors propagated from internal calls;#### Parameters/#### Returnsonly where they add detail beyond the one-line summary (a trivial getter needs neither);assert!(diff < epsilon)) stay bound-based — do not convert toassert_eq!.access(delayed/two_stepBorrow,two_stepRequestBorrow) andfixed_pointComponents.test_prefix fromaccess_control_testsfunctions (descriptive names per STYLEGUIDE Testing), matching the sibling test modules.Verification
sui move test --build-env testnet --lint --warnings-are-errorsandsui move build --doc --build-env testnet, all green:contracts/access— 137 testsmath/core— 753 testsmath/fixed_point— 426 testsOut of scope (follow-up)
access_control_testsstill carries#[allow(lint(...))](4) andabort 999sentinels (43); removing them requires restructuring the expected-failure tests (and resolvingshare_owned) and is left to a separate focused PR.🤖 Generated with Claude Code
Summary by CodeRabbit
Aborts/error documentation and caller preconditions.