Skip to content

Allow the user to select eyre or anyhow as error backend#33

Draft
yozhgoor wants to merge 10 commits intomainfrom
eyre
Draft

Allow the user to select eyre or anyhow as error backend#33
yozhgoor wants to merge 10 commits intomainfrom
eyre

Conversation

@yozhgoor
Copy link
Copy Markdown
Member

No description provided.

@yozhgoor yozhgoor marked this pull request as draft April 15, 2026 10:29
@yozhgoor yozhgoor requested review from cecton April 15, 2026 10:29
Copy link
Copy Markdown
Member

@cecton cecton left a comment

Choose a reason for hiding this comment

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

Overall this is a clean and well-structured PR. A few things worth discussing:

: Missing closing backtick in the toml doc block

In both (line ~93) and (line ~107), the new toml code block showing the dependency is missing its closing ``` fence. This will cause the rendered docs/README to swallow the following section headers and content.

: Repeated blocks could be factored out

The pattern of duplicating / blocks in two separate closures (for and ) is verbose. Both crates provide error context in the same logical way - consider a small helper macro or trait extension in a private module to unify the two backends behind a single call. This would also make future calls easier.

: bumps MSRV to 1.85, which is a breaking change for users

The bump from to is significant. Since the PR description doesn't mention this as intentional, it's worth calling out explicitly - it's a semver-compatible change in this crate's policy but it may surprise downstream users. If the edition bump is primarily needed for support, it would be good to document that in the PR/changelog.

: heading level regression

The section was demoted to (h1) while its subsections remain . This inconsistency may render oddly. It looks like it was accidentally changed from to during the refactor.

: flag dropped

The original
running 2 tests
test test::command_list_froms ... ok
test test::exclude_relative_path ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.05s

running 3 tests
test src/lib.rs - (line 89) - compile ... ok
test src/lib.rs - (line 143) ... ok
test src/lib.rs - (line 152) ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.10s included workspace members; the new
running 2 tests
test test::command_list_froms ... ok
test test::exclude_relative_path ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

running 3 tests
test src/lib.rs - (line 89) - compile ... ok
test src/lib.rs - (line 152) ... ok
test src/lib.rs - (line 143) ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.10s and omit . If there are workspace members (e.g., ), they will no longer be tested in CI. Was this intentional?

Minor nit: the guards enforcing mutual exclusivity are a nice touch - appreciated.

Copy link
Copy Markdown
Member

@cecton cecton left a comment

Choose a reason for hiding this comment

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

(Repost of previous review — the body was mangled by shell backtick expansion. Please disregard the garbled comment above.)

Overall this is a clean and well-structured PR. A few things worth discussing:


Cargo.toml / README.md / src/lib.rs: Missing closing fence in the toml doc block

In both src/lib.rs (around line 93) and README.md (around line 107), the new toml code block showing the eyre dependency is missing its closing triple-backtick fence. This will cause the rendered docs/README to swallow the following section headers and content into the code block.


src/lib.rs: Repeated #[cfg] blocks could be factored out

The pattern of duplicating with_context / wrap_err_with in two nearly-identical .map() closures (for exclude_paths and watch_paths) is verbose. Both crates provide error context in the same logical way — consider a small private helper or a trait extension to unify the two backends behind a single call. This would reduce noise and make future additions easier.


Cargo.toml: edition = "2024" bumps MSRV to 1.85, which is a potentially breaking change for users

The rust-version bump from 1.78 to 1.85.1 is significant. Since the PR description doesn't mention this as intentional, it's worth calling out explicitly. If the edition bump is primarily needed for eyre support or unrelated cleanup, it would be good to document that in the PR body or a changelog entry.


README.md: Heading level regression in the Examples section

The ## Examples heading was changed to # Examples (h1) while its subsections remain ##. This inconsistency will likely render oddly. Looks like it was accidentally changed from ## to # during the refactor.


.github/workflows/rust.yml: --workspace flag dropped from cargo test

The original step ran cargo test --workspace; the new steps run cargo test and cargo test --no-default-features --features eyre without --workspace. If there are workspace members (e.g. examples/demo), they will no longer be tested in CI. Was this intentional?


Minor nit: the compile_error! guards enforcing mutual exclusivity of anyhow and eyre are a nice touch.

@yozhgoor
Copy link
Copy Markdown
Member Author

yozhgoor commented Apr 15, 2026

--workspace flag dropped from cargo test

Workspace members and examples are not related. We don't have any workspace member for this project so I removed it.

Also, we are not using the examples directory as it was designed but I can make it work with --examples demo if you want with something like this:

[[example]]
name = "demo"
path = "examples/demo/xtask/src/main.rs"

@yozhgoor
Copy link
Copy Markdown
Member Author

edition = "2024" bumps MSRV to 1.85, which is a potentially breaking change for users

It is, but the tests for the MSRV version are failing because of the following error:

error: failed to parse manifest at `/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_lex-1.1.0/Cargo.toml`

Caused by:
  feature `edition2024` is required

  The package requires the Cargo feature called `edition2024`, but that feature is not stabilized in this version of Cargo

So for me it was a good opportunity to upgrade to the latest edition.

@yozhgoor yozhgoor requested a review from cecton April 15, 2026 17:52
@cecton
Copy link
Copy Markdown
Member

cecton commented Apr 18, 2026

I don't know if this is a good idea. We would need to do the same for xtask-wasm and I don't really want to bother.

Okay to close it?

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.

3 participants