cargo icon indicating copy to clipboard operation
cargo copied to clipboard

"cargo new" should add pre-commit hooks for autoformatter and linter

Open worldmind opened this issue 1 year ago • 7 comments

Problem

  1. Usage of autoformatter and linter is a really good practice and, for not depend on IDE, good to have them in pre-commit hooks (of course it can also be some rust implementation (if any)) for block commiting changes which has issues.
  2. Rust/cargo has rustfmt and clippy.
  3. cargo new even does initialization of new git repo, has all tools, but doesn't configure pre-commit hooks, but why to not add them as it clearly good practice?

Proposed Solution

cargo new should depends on pre-commit or something similar and add config (.pre-commit-config.yaml for pre-commit).

Notes

For rustfmt I found example here:

repos:
  - repo: local
    hooks:
      - id: rustfmt
        name: rustfmt
        description: Check if all files follow the rustfmt style
        entry: cargo fmt --all -- --check --color always
        language: system
        pass_filenames: false

another example, plus example for clippy can be found here:

-   repo: local
    hooks:
    -   id: rust-linting
        name: Rust linting
        description: Run cargo fmt on files included in the commit. rustfmt should be installed before-hand.
        entry: cargo fmt --all --
        pass_filenames: true
        types: [file, rust]
        language: system
    -   id: rust-clippy
        name: Rust clippy
        description: Run cargo clippy on files included in the commit. clippy should be installed before-hand.
        entry: cargo clippy --all-targets --all-features -- -Dclippy::all
        pass_filenames: false
        types: [file, rust]
        language: system

worldmind avatar Apr 15 '24 18:04 worldmind

A pre-commit hook is fairly opinionated choice

  • It is useless unless the user has the commit hook program installed
  • It is useless unless the hooks get registered (see above)
  • iirc commit hooks are not composable unless you completely buy into one specific pre-commit hook tool

Also, for myself, one of the most frustrating thing about project scaffolding tools is all of the content you have to delete. I'd rather not be generating this content unless its going to be used or else we are pushing for people to delete it.

imo this seems better suited to be solved by either

  • #5420
  • #5151

epage avatar Apr 15 '24 18:04 epage

  1. About pre-commit: IMO it's just kind of standard tool for git hooks, so, shouldn't be big issue to install it as a cargo dependency and run pre-commit install, but I think git hooks can be added without it if any concerns about dependency.
  2. Agree that in many cases default setup is not that useful, but it's bigger topic about defaults in general. At least for now cargo new's defaults looks good for new starters and it's good. So, if defaults are for people who starting learn Rust might be good to show all best practices.

But it just an idea, thank you for nice tooling!

worldmind avatar Apr 15 '24 19:04 worldmind

Just an additional example of hooks found here.

ashrub-holvi avatar Apr 17 '24 11:04 ashrub-holvi

From the previous discussion https://github.com/rust-lang/cargo/issues/12102#issuecomment-1540686779, the team generally leans toward adding extra VCS support via some extension points in Cargo, to lighten the maintenance burden. Adding new features for existing VCS support falls into the same category as that, I think.

weihanglo avatar Apr 18 '24 02:04 weihanglo

Rustup's minimal profile does not include rustfmt or clippy. I do not really believe Cargo should add anything that assumes or significantly varies its user experience based on whether it ships with tools that do not appear in the minimal profile.

workingjubilee avatar Apr 25 '24 20:04 workingjubilee

...My opinion is admittedly also informed by the fact that I run git commit --amend about 4 times per commit and rebase every PR about twice before I push it, and clippy and rustfmt can contain algorithms with a performance worse than O(n) (partly because they use the Rust compiler, which has such algorithms already).

workingjubilee avatar Apr 25 '24 21:04 workingjubilee

whether it ships with tools that do not appear in the minimal profile.

That's a pretty fair point! Cargo also has no knowledge about rustup profiles. At least Cargo needs extra executable probes to find their existence.

weihanglo avatar Apr 25 '24 22:04 weihanglo