est31
Minor repo cleanup proposal
Hi, thanks for the awesome tool! I use it regularly, and would be happy to contribute. Would it be ok to update the repo a bit to bring it inline with the common rust projects, e.g. with the standard code formatting and clippy validation? I suspect many of these will also help onboard this into cargo in the future. Some of the ideas:
- run
cargo fmt --all-- this ensures every contributor uses the same standard rust format, and minimizes the diffs - run
cargo clippyand fix all issues -- i discovered many bugs with this in numerous projects - introduce
[lints]section to Cargo.toml to improve code quality - introduce MSRV
- add clippy enforcement to CI
- enable https://pre-commit.ci/ to automatically fix minor formatting and other text issues without the user involvement (i.e. auto-fix all PRs)
- enable codecoverage like https://codecov.io and upload coverage results there
- ...
Thanks for the offer, but I'd consider this project mostly done. Of course there is bugs, limitations, potential future features, and contributions are totally welcome, but I don't see much of a point in putting major effort into it given that the best place is upstream.
The blocker for upstreaming is a single invocation that checks all compilation units, i.e. also doctests: https://github.com/rust-lang/cargo/issues/6669#issuecomment-2326764167 . Which at least there has been interest from in the cargo team, whether this means that rust will get it, idk.
many of these will also help onboard this into cargo in the future.
fwiw I already had a cargo PR five years ago. I would base future upstreaming efforts on that PR instead of this tool.
cargo fmt
I agree it makes contributions easier, but if there is cargo fmt, it means I'd need to run it regularly before making commits, which would add friction. Right now the repo has little outside contributions, so if it required me to run it, it would actually hurt the contributors to cargo-udeps (mostly me) more than it would help them.
clippy, clippy enforcement
I think correctness lints can be checked and enforced, but some of the style lints are a bit weird.
MSRV
actually good point, we might be able to add it. but our hands are tied with cargo's policy being that only the last released rust version is supported, and we ourselves want to update the cargo crate as soon as possible. In other words, the MSRV is bound by cargo, which is, at least for released cargo versions, "latest stable minus one".
pre-commit.ci, coverage, ...
Cool to play around with them but I don't really have the time for those given my full time job :/. Also, for people new to git, it might actually make things worse when there is something that changes upstream, and they have to sync commits from their branch.
@est31 thx for such a detailed reply! There are a few things I think I did not explain that lead to the wrong conclusions:
pre-commit.ci, coverage, ...
Cool to play around with them but I don't really have the time for those given my full time job :/. Also, for people new to git, it might actually make things worse when there is something that changes upstream, and they have to sync commits from their branch.
cargo fmt
I agree it makes contributions easier, but if there is
cargo fmt, it means I'd need to run it regularly before making commits, which would add friction. Right now the repo has little outside contributions, so if it required me to run it, it would actually hurt the contributors to cargo-udeps (mostly me) more than it would help them.
Sadly, pre-commit.ci is a misnomer. This is NOT git pre-commit hook installed by all users. Just like any other CI step, this well known service runs whenever a contributor creates a PR, does minor fixes (like cargo fmt, crlf->lf, and many other) and pushes another commit directly into the PR as if contributed by the user. I created this PR with my default config against my own fork (I have precommit enabled for all my repos) -- it automatically formatted the code and pushed the change. So in short - noone will ever need to worry about cargo fmt - it happens automatically before you accept the change.
- https://github.com/nyurik/cargo-udeps/pull/1
- https://github.com/nyurik/cargo-udeps/pull/1/commits/daecbc20a348d47ad799052f9c16b04e845f0baa -- config
- https://github.com/nyurik/cargo-udeps/pull/1/commits/d6bb4fe830a65cc92d1550e3e21618f159c1880a -- automatic fixes (make sure to enable
hide whitespacewhen viewing)
Thanks for the offer, but I'd consider this project mostly done. Of course there is bugs, limitations, potential future features, and contributions are totally welcome, but I don't see much of a point in putting major effort into it given that the best place is upstream.
This is actually not a problem - I can do the commit in half an hour - I maintain 25+ projects with near-identical settings for many years - so not an issue to port.
The blocker for upstreaming is a single invocation that checks all compilation units, i.e. also doctests: rust-lang/cargo#6669 (comment) . Which at least there has been interest from in the cargo team, whether this means that rust will get it, idk.
many of these will also help onboard this into cargo in the future.
fwiw I already had a cargo PR five years ago. I would base future upstreaming efforts on that PR instead of this tool.
good to know, thanks!
clippy, clippy enforcement
I think correctness lints can be checked and enforced, but some of the style lints are a bit weird.
heh, well, easy enough to disable if not needed :)
MSRV
actually good point, we might be able to add it. but our hands are tied with cargo's policy being that only the last released rust version is supported, and we ourselves want to update the cargo crate as soon as possible. In other words, the MSRV is bound by
cargo, which is, at least for released cargo versions, "latest stable minus one".
totally up to you, this is clearly not a biggie