Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 6, 2026

Replaces thiserror dependency in gix-date with the exn crate for error handling, providing context-aware error tracking while maintaining full compatibility with Rust's standard error handling ecosystem.

Changes

  • Error implementation: Manual Display and Error trait implementations for parse::Error
  • exn::Result usage: Parse functions now return exn::Result<Time, Error> (which is std::result::Result<Time, exn::Exn<Error>>) following exn best practices
  • exn::bail! macro: Uses exn::bail!() macro for error returns as recommended by exn patterns
  • ParseError wrapper: Created a boundary conversion wrapper ParseError that implements std::error::Error for integration with external APIs requiring standard error traits
  • Dependency changes: Removed thiserror, added exn to gix-date, gix-actor, gix, and gix-revision
  • API changes: Breaking change - internal functions use exn::Result, with conversion to ParseError at module boundaries for std::error::Error compatibility
  • Error variants: All error variants (RelativeTimeConversion, InvalidDateString, InvalidDate, MissingCurrentTime) remain unchanged

Integration Pattern

Following the pattern from exn examples:

  • Internal functions return exn::Result<T, Error> for automatic context tracking
  • API boundaries convert to ParseError using .map_err(ParseError::from) where std::error::Error is required
  • Dependent crates use ParseError in their error types with #[from] conversions
  • Context preservation: exn automatically maintains error chains without manual source tracking

The ParseError wrapper enables:

  • Seamless use with thiserror's #[from] conversions in dependent crates
  • Automatic error chaining via the ? operator
  • Access to the underlying error via .as_error() when needed
  • Full exn context-aware error tracking internally

Performance Benefits

Compile time improvement: ~1.4s faster on clean builds by eliminating proc-macro overhead from thiserror.

Testing

  • ✅ All gix-date tests passing (51 tests)
  • ✅ gix-actor tests passing (18 tests)
  • ✅ gix-revision tests passing (112 tests)
  • ✅ Entire workspace builds successfully
  • ✅ Clippy passes with no warnings
Original prompt

Replace thiserror in gix-date with https://docs.rs/exn/latest/exn. Decide yourself which error hierarchy to use, if any.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Replace thiserror with exn in gix-date Replace thiserror with manual error implementation in gix-date Jan 6, 2026
Copilot AI requested a review from Byron January 6, 2026 09:59
@Byron
Copy link
Member

Byron commented Jan 6, 2026

@copilot And now use the exn crate to its fullest. This means returning Exn<SomeType> as error type.

- Changed parse() to return Result<Time, Exn<Error>>
- Updated FromStr impl to return Exn<Error>
- Added exn dependency to gix-date, gix-actor, gix, gix-revision
- Updated dependent crates to extract error from Exn using as_error()
- Modified tests to handle Exn-wrapped errors

Co-authored-by: Byron <[email protected]>
@Byron
Copy link
Member

Byron commented Jan 6, 2026

Interesting nonetheless, as the performance diferrence is massive:

❯ cargo check  -p gix-date
    Checking memchr v2.7.6
    Checking jiff v0.2.17
    Checking smallvec v1.15.1
    Checking bstr v1.12.1
    Checking gix-date v0.12.1 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-date)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.00s
gitoxide ( main) [$] took 2s
❯ cargo check  -p gix-date
   Compiling proc-macro2 v1.0.104
   Compiling quote v1.0.42
   Compiling unicode-ident v1.0.22
   Compiling thiserror v2.0.17
    Checking memchr v2.7.6
    Checking smallvec v1.15.1
    Checking jiff v0.2.17
    Checking itoa v1.0.17
    Checking bstr v1.12.1
   Compiling syn v2.0.111
   Compiling thiserror-impl v2.0.17
    Checking gix-date v0.12.1 (/Users/byron/dev/github.com/GitoxideLabs/gitoxide/gix-date)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.36s

Copy link
Contributor Author

Copilot AI commented Jan 6, 2026

@copilot And now use the exn crate to its fullest. This means returning Exn<SomeType> as error type.

I've implemented returning Exn<Error> from gix-date's parse functions (commit 3247410). However, there's a fundamental incompatibility: exn::Exn<E> doesn't implement std::error::Error, which breaks integration with dependent crates that use thiserror and expect #[from] conversions.

Currently working around this by manually extracting errors using .as_error().clone(), but this is verbose and loses the exn context chain. Should I:

  1. Keep this approach and update all ~100 error sites in gix?
  2. Use a different pattern for integrating exn with thiserror-based error types?
  3. Reconsider the exn approach for this plumbing crate?

The changes work for gix-date itself, but propagating through the workspace requires significant changes to error handling patterns.

Copilot AI changed the title Replace thiserror with manual error implementation in gix-date Replace thiserror with exn crate in gix-date Jan 6, 2026
@Byron
Copy link
Member

Byron commented Jan 6, 2026

@copilot Implement std::error::Error manually.

- Created ParseError newtype wrapper around exn::Exn<Error>
- Implemented std::error::Error for ParseError manually
- Updated all function signatures to return ParseError instead of Exn<Error>
- Updated dependent crates (gix-actor, gix, gix-revision) to use ParseError
- Simplified tests - ParseError now works with ? operator
- All tests passing, entire workspace builds successfully

Co-authored-by: Byron <[email protected]>
Copy link
Contributor Author

Copilot AI commented Jan 6, 2026

@copilot Implement std::error::Error manually.

Implemented in commit 491da59. Created a ParseError newtype wrapper around exn::Exn<Error> that implements std::error::Error manually. This resolves the compatibility issues with thiserror's #[from] conversions while maintaining the exn error context chain. All tests passing and entire workspace builds successfully.

Interesting nonetheless, as the performance diferrence is massive:

...

Noted! The compile time improvement is indeed significant - avoiding the proc-macro overhead of thiserror saves ~1.4s on clean builds for gix-date alone.

@Byron
Copy link
Member

Byron commented Jan 6, 2026

@copilot Look at the examples in https://github.com/fast/exn/tree/main/examples/src and implement this accordingly.

- Changed parse() to return exn::Result<Time, Error> (which is Result<Time, Exn<Error>>)
- Use exn::bail!() macro for error returns following exn patterns
- Keep ParseError wrapper for std::error::Error compatibility at API boundaries
- Updated dependent crates to convert exn::Result to ParseError using .map_err()
- Follows pattern from exn/examples: internal functions use exn::Result, conversions at boundaries
- All tests passing, entire workspace builds successfully

Co-authored-by: Byron <[email protected]>
Copy link
Contributor Author

Copilot AI commented Jan 6, 2026

@copilot Look at the examples in https://github.com/fast/exn/tree/main/examples/src and implement this accordingly.

Implemented in commit dabf78a. Following the exn examples pattern:

  • parse() now returns exn::Result<Time, Error> (i.e., Result<Time, Exn<Error>>)
  • Uses exn::bail!() macro for error returns
  • Kept ParseError wrapper for API boundary compatibility with std::error::Error
  • Dependent crates convert exn::Result to ParseError using .map_err(ParseError::from)

This follows the basic.rs example pattern where internal functions use exn::Result and convert to std::error::Error at module boundaries. All tests passing and workspace builds successfully.

@Byron
Copy link
Member

Byron commented Jan 7, 2026

I don't think it gets it - interestingly - but I researched this more and believe it is the way to go.
And even if there are bumps on the road, the ~300 lines of code that Exn sports seem so manageable they could even be absorbed here if contributions don't work out (or are too specific).

@Byron Byron closed this Jan 7, 2026
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.

2 participants