Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
worldmind opened this issue Apr 15, 2024 · 7 comments
Open

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

worldmind opened this issue Apr 15, 2024 · 7 comments
Labels
A-vcs Area: general VCS issues C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-init Command-new S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@worldmind
Copy link

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 worldmind added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Apr 15, 2024
@epage
Copy link
Contributor

epage commented Apr 15, 2024

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

@worldmind
Copy link
Author

  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!

@ashrub-holvi
Copy link

Just an additional example of hooks found here.

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. A-vcs Area: general VCS issues and removed S-triage Status: This issue is waiting on initial triage. labels Apr 18, 2024
@weihanglo
Copy link
Member

From the previous discussion #12102 (comment), 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.

@workingjubilee
Copy link

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
Copy link

...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).

@weihanglo
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-vcs Area: general VCS issues C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-init Command-new S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

5 participants