docs(guide/dev): refine error handling guidelines for derive_more traits#366
Merged
Conversation
`ParsedValue` is not an error type — it's a display wrapper for formatted byte values. Remove the unused `Error` derive and update documentation to clarify that `Display` and `Error` should only be derived when actually used. https://claude.ai/code/session_012YdPMpQ3UiSmqFSU2vUULm
Update CONTRIBUTING.md and AI instruction templates to clarify that `Display` and `Error` should only be derived when each trait is actually used, rather than always bundling them together as `#[derive(Debug, Display, Error)]`. https://claude.ai/code/session_012YdPMpQ3UiSmqFSU2vUULm
Contributor
There was a problem hiding this comment.
Pull request overview
Refines developer guidance to clarify when Rust types should derive Display and/or std::error::Error via derive_more, aiming to avoid unnecessary trait implementations while keeping error-handling patterns consistent across the repo’s docs and AI instruction files.
Changes:
- Expanded
CONTRIBUTING.mderror-handling guidance to differentiate when to deriveDisplayvsError. - Updated AI instruction documents to replace the blanket “derive
Debug, Display, Error” recommendation with conditional guidance. - Aligned the shared AI-instructions template fragment with the updated recommendations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
template/ai-instructions/shared.md |
Updates the shared “Quick Reference” bullet for error trait derivation guidance. |
CONTRIBUTING.md |
Expands the “Error Handling” section with more nuanced derive guidance. |
CLAUDE.md |
Updates the “Quick Reference” bullet for error trait derivation guidance. |
AGENTS.md |
Updates the “Quick Reference” bullet for error trait derivation guidance. |
.github/copilot-instructions.md |
Updates the “Quick Reference” bullet for error trait derivation guidance. |
Performance Regression Reportscommit: cced310 There are no regressions. |
`std::error::Error` requires `Display` as a supertrait, so saying "not all error types need Display" was inaccurate. Simplify the guideline to focus on the valid point: not all displayable types are errors. https://claude.ai/code/session_012YdPMpQ3UiSmqFSU2vUULm
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
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.
Summary
Updated error handling documentation across all guidance files to clarify when to derive
DisplayandErrortraits fromderive_more, emphasizing that not all types need both traits.Key Changes
DisplayvsError, including examples of types that only needDisplayfor formatting purposes#[derive(Debug, Display, Error)]recommendation with conditional trait derivationNotable Details
The changes clarify that:
Displayshould be derived when a type needs to be displayed (e.g., printed to stderr, used in format strings)Errorshould be derived when the type is used asstd::error::Error(e.g., as aResulterror type or as a source of another error)DisplaywithoutError, avoiding unnecessary trait implementationshttps://claude.ai/code/session_012YdPMpQ3UiSmqFSU2vUULm