Skip to content

Comments

fix(ohno): address some correctness and usability findings#278

Open
geeknoid wants to merge 1 commit intomainfrom
geeknoid/ohno
Open

fix(ohno): address some correctness and usability findings#278
geeknoid wants to merge 1 commit intomainfrom
geeknoid/ohno

Conversation

@geeknoid
Copy link
Member

@geeknoid geeknoid commented Feb 20, 2026

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 IntoAppError implementations

Usability:

  • Make enrichments()/enrichment_messages() iterate in chronological order (oldest-first), consistent with format_error display order

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in app_err! macro
  • Correctness: Fixed field name generation bug and added compile error for enums, plus #[track_caller] for Option<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.

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 geeknoid changed the title fix(ohno): address performance, correctness, and usability findings fix(ohno): address some correctness and usability findings Feb 20, 2026
@codecov
Copy link

codecov bot commented Feb 20, 2026

Codecov Report

❌ Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.9%. Comparing base (bc20e71) to head (6f8cf37).

Files with missing lines Patch % Lines
crates/ohno_macros/src/error_type_attr.rs 92.8% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #278     +/-   ##
========================================
- Coverage   100.0%   99.9%   -0.1%     
========================================
  Files         141     141             
  Lines        8640    8653     +13     
========================================
+ Hits         8640    8652     +12     
- Misses          0       1      +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant