Conversation
cecton
left a comment
There was a problem hiding this comment.
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.
cecton
left a comment
There was a problem hiding this comment.
(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.
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 [[example]]
name = "demo"
path = "examples/demo/xtask/src/main.rs" |
It is, but the tests for the MSRV version are failing because of the following error: So for me it was a good opportunity to upgrade to the latest edition. |
|
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? |
No description provided.