fix(ohno): address some correctness and usability findings#278
Open
fix(ohno): address some correctness and usability findings#278
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses performance, correctness, and usability issues in the ohno error handling library. The changes improve macro efficiency, fix a field naming bug, add compile-time safety, ensure proper call-site tracking, and make error enrichment iteration order consistent with display order.
Changes:
- Performance: Removed unnecessary
format!()allocation for plain string literals inapp_err!macro - Correctness: Fixed field name generation bug and added compile error for enums, plus
#[track_caller]forOption<T>methods - Usability: Made enrichment iterators return in chronological order (oldest-first), matching the display format
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ohno_macros/src/utils.rs | Fixed generate_unique_field_name to avoid compounding names; added test for multiple collision scenario |
| crates/ohno_macros/src/error_type_attr.rs | Changed to return error when #[ohno::error] is applied to enums; updated tests accordingly |
| crates/ohno/tests/enrich_err.rs | Updated test expectations to reflect chronological ordering of enrichments |
| crates/ohno/tests/core.rs | Updated test expectations to reflect chronological ordering of enrichments |
| crates/ohno/src/core.rs | Removed .rev() from enrichment iterators to return chronological order; updated documentation |
| crates/ohno/src/app/macros.rs | Performance optimization: removed format!() for plain string literals |
| crates/ohno/src/app/into_app_err.rs | Added #[track_caller] to Option<T> implementations for correct location tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
geeknoid
force-pushed
the
geeknoid/ohno
branch
2 times, most recently
from
73dfe6d to
6f8cf37
Compare
geeknoid
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 141 141
Lines 8640 8670 +30
=======================================
+ Hits 8640 8670 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
geeknoid
force-pushed
the
geeknoid/ohno
branch
2 times, most recently
from
6f7bff6 to
ac51d96
Compare
Correctness: - Fix compounding name bug in generate_unique_field_name (ohno_core_1_2 instead of ohno_core_2 on repeated collision) - Emit clear compile error when #[ohno::error] is applied to enums - Add #[track_caller] to Option<T> IntoAppError implementations Usability: - Make enrichments()/enrichment_messages() iterate in chronological order (oldest-first), consistent with format_error display order
geeknoid
force-pushed
the
geeknoid/ohno
branch
from
ac51d96 to
3245641
Compare
martin-kolinek
approved these changes
Feb 24, 2026
| /// Returns an iterator over the enrichment information in chronological order (oldest first). | ||
| pub fn enrichments(&self) -> impl Iterator<Item = &EnrichmentEntry> { | ||
| self.data.enrichment.iter().rev() | ||
| self.data.enrichment.iter() |
Collaborator
There was a problem hiding this comment.
@Vaiz, this looked intentional in the original version - what was the reason to go against the format_error! convention?
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| ohno::bail!("cargo metadata failed: {stderr}"); | ||
| ohno::bail!("cargo metadata failed: {}", stderr); |
Collaborator
There was a problem hiding this comment.
I'm a bit surprised this doesn't cause a clippy warning.
Member
Author
There was a problem hiding this comment.
I'm guessing clippy doesn't see this as a formatting thing.
tekian
approved these changes
Feb 24, 2026
sandersaares
approved these changes
Feb 25, 2026
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
4 participants
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.
Correctness:
Usability: