CLAUDE.md: error handling + test instructions updates#412
Open
malcolmgreaves wants to merge 1 commit intomainfrom
Open
CLAUDE.md: error handling + test instructions updates#412malcolmgreaves wants to merge 1 commit intomainfrom
malcolmgreaves wants to merge 1 commit intomainfrom
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughDocumentation-only change to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 @.claude/CLAUDE.md:
- Line 105: The sentence at Line 105 is malformed and ambiguous: replace the
broken fragment ".on a `Result`" with the intended method reference (for example
".expect() on `Result`") so the rule reads clearly (e.g., "Avoid using
`.unwrap()` or `.expect()` on a `Result`. It is only OK to do this within a test
case."). Update the rule text to explicitly mention the correct method names
(`.unwrap()`, `.expect()`) and reference `Result` as the type to remove the
broken code span and ambiguity.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92fea7f0-d23e-47dc-b9ee-1b43f85d40c4
📒 Files selected for processing (1)
.claude/CLAUDE.md
malcolmgreaves
force-pushed
the
mg/claude_settings_tweaks
branch
2 times, most recently
from
8f2824a to
9aaff6f
Compare
Updated the `CLAUDE.md` file to expound on error handling conventions. `OxenError` is top-level unifying type. Encourage to use specific errors and wrap in `OxenError` variants with `#[from]` conversions. Included instruction to ensure the `oxen-server` server is running for tests.
malcolmgreaves
force-pushed
the
mg/claude_settings_tweaks
branch
from
9aaff6f to
5c3102e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/CLAUDE.md:
- Line 106: Fix the typo in the test-only code note: change the misspelled word
"descrptive" to "descriptive" inside the phrase `.expect(<descrptive explanation
of invariant that was violated>)` so it reads `.expect(<descriptive explanation
of invariant that was violated>)` in the test exception note.
- Line 107: In the sentence that reads "Use as specific of an error type as
possible for a function. Don't use a wider type unless it's necessary. When
making modules and related pieces of code, try to use a locally-defined error
enum for them if they all have similiar errors." fix the typo by replacing the
word "similiar" with the correct spelling "similar" so the final sentence reads
"...if they all have similar errors." and keep the surrounding guidance
unchanged.
- Line 108: Fix the typos in the guidance sentence so it reads correctly: change
"a `OxenError` variant" to "an `OxenError` variant" and correct "similiar" to
"similar" in the same line that references wrapping other modules' error types
and using `Box<>` with `#[from]`; keep the rest of the guidance unchanged and
ensure backticks around `OxenError`, `Box<>`, and `#[from]` remain intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b1f96ff-6348-4a6c-9a2f-7e458183c3d3
📒 Files selected for processing (1)
.claude/CLAUDE.md
| - Implement proper error propagation through the `?` operator | ||
| - Use the result type (`Result<T, Error>`) when an operation could fail. | ||
| - Never use `.unwrap()` or `.expect()` on a `Result` or on an `Option`. | ||
| + Exception: In test-only code, it is ok to use use `.expect(<descrptive explanation of invariant that was violated>)` since we want to fail fast and have good stack traces for failing test cases. |
Contributor
There was a problem hiding this comment.
Fix typo in test exception note.
The word "descrptive" is misspelled.
✏️ Proposed fix
- + Exception: In test-only code, it is ok to use use `.expect(<descrptive explanation of invariant that was violated>)` since we want to fail fast and have good stack traces for failing test cases.
+ + Exception: In test-only code, it is ok to use `.expect(<descriptive explanation of invariant that was violated>)` since we want to fail fast and have good stack traces for failing test cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/CLAUDE.md at line 106, Fix the typo in the test-only code note:
change the misspelled word "descrptive" to "descriptive" inside the phrase
`.expect(<descrptive explanation of invariant that was violated>)` so it reads
`.expect(<descriptive explanation of invariant that was violated>)` in the test
exception note.
| - Use the result type (`Result<T, Error>`) when an operation could fail. | ||
| - Never use `.unwrap()` or `.expect()` on a `Result` or on an `Option`. | ||
| + Exception: In test-only code, it is ok to use use `.expect(<descrptive explanation of invariant that was violated>)` since we want to fail fast and have good stack traces for failing test cases. | ||
| - Use as specific of an error type as possible for a function. Don't use a wider type unless it's necessary. When making modules and related pieces of code, try to use a locally-defined error enum for them if they all have similiar errors. |
Contributor
There was a problem hiding this comment.
Fix typo in error type guidance.
"similiar" is misspelled.
✏️ Proposed fix
-- Use as specific of an error type as possible for a function. Don't use a wider type unless it's necessary. When making modules and related pieces of code, try to use a locally-defined error enum for them if they all have similiar errors.
+- Use as specific of an error type as possible for a function. Don't use a wider type unless it's necessary. When making modules and related pieces of code, try to use a locally-defined error enum for them if they all have similar errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/CLAUDE.md at line 107, In the sentence that reads "Use as specific
of an error type as possible for a function. Don't use a wider type unless it's
necessary. When making modules and related pieces of code, try to use a
locally-defined error enum for them if they all have similiar errors." fix the
typo by replacing the word "similiar" with the correct spelling "similar" so the
final sentence reads "...if they all have similar errors." and keep the
surrounding guidance unchanged.
| - Never use `.unwrap()` or `.expect()` on a `Result` or on an `Option`. | ||
| + Exception: In test-only code, it is ok to use use `.expect(<descrptive explanation of invariant that was violated>)` since we want to fail fast and have good stack traces for failing test cases. | ||
| - Use as specific of an error type as possible for a function. Don't use a wider type unless it's necessary. When making modules and related pieces of code, try to use a locally-defined error enum for them if they all have similiar errors. | ||
| - Make sure there's a `OxenError` variant for every error type. Be liberal in wrapping other modules error types, or other specific error types, in a new variant. Use a `Box<>` wrapper for it and have a `#[from]` to derive. |
Contributor
There was a problem hiding this comment.
Fix typos in OxenError variant guidance.
Two issues on this line:
- Article should be "an" before
OxenError(vowel sound) - "similiar" is misspelled
✏️ Proposed fixes
-- Make sure there's a `OxenError` variant for every error type. Be liberal in wrapping other modules error types, or other specific error types, in a new variant. Use a `Box<>` wrapper for it and have a `#[from]` to derive.
+- Make sure there's an `OxenError` variant for every error type. Be liberal in wrapping other modules error types, or other specific error types, in a new variant. Use a `Box<>` wrapper for it and have a `#[from]` to derive.-- Make sure there's a `OxenError` variant for every error type. Be liberal in wrapping other modules error types, or other specific error types, in a new variant. Use a `Box<>` wrapper for it and have a `#[from]` to derive.
+- Make sure there's an `OxenError` variant for every error type. Be liberal in wrapping other modules error types, or other specific error types, in a new variant. Use a `Box<>` wrapper for it and have a `#[from]` to derive.Actually, there's also "similiar" that needs fixing:
-- Use as specific of an error type as possible for a function. Don't use a wider type unless it's necessary. When making modules and related pieces of code, try to use a locally-defined error enum for them if they all have similiar errors.
+- Use as specific of an error type as possible for a function. Don't use a wider type unless it's necessary. When making modules and related pieces of code, try to use a locally-defined error enum for them if they all have similar errors.Combined fix:
-- Make sure there's a `OxenError` variant for every error type. Be liberal in wrapping other modules error types, or other specific error types, in a new variant. Use a `Box<>` wrapper for it and have a `#[from]` to derive.
+- Make sure there's an `OxenError` variant for every error type. Be liberal in wrapping other modules error types, or other specific error types, in a new variant. Use a `Box<>` wrapper for it and have a `#[from]` to derive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/CLAUDE.md at line 108, Fix the typos in the guidance sentence so it
reads correctly: change "a `OxenError` variant" to "an `OxenError` variant" and
correct "similiar" to "similar" in the same line that references wrapping other
modules' error types and using `Box<>` with `#[from]`; keep the rest of the
guidance unchanged and ensure backticks around `OxenError`, `Box<>`, and
`#[from]` remain intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated the
CLAUDE.mdfile to expound on error handling conventions.OxenErroris top-level unifying type. Encourage to use specific errors andwrap in
OxenErrorvariants with#[from]conversions.Included instruction to ensure the
oxen-serverserver is running for tests.