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

Identify rust-toolchain files as rust-toolchain #459

Closed
wants to merge 1 commit into from

Conversation

jalaziz
Copy link
Contributor

@jalaziz jalaziz commented May 12, 2024

Introduce dedicated types for rust-toolchain and rust-toolchain.toml files. Cargo linting rules can change with the rust version, so it's helpful to match against the rust-toolchain files as a trigger for Rust linting hooks.

Introduce dedicated types for rust-toolchain and rust-toolchain.toml
files. Cargo linting rules can change with the rust version, so it's
helpful to match against the rust-toolchain files as a trigger for Rust
linting hooks.
@asottile
Copy link
Member

same feedback as your other PR: #460 (comment)

@jalaziz
Copy link
Contributor Author

jalaziz commented May 30, 2024

same feedback as your other PR: #460 (comment)

Yes, filtering by files can accomplishes the same things, but it's impossible to combine files and types with OR.

The docs encourage using types with:

Filtering with types provides several advantages over traditional filtering with files.

  • no error-prone regular expressions
  • files can be matched by their shebang (even when extensionless)
  • symlinks / submodules can be easily ignored

Which encourages the use of types like rust, cargo, etc.

However, due to:

types, types_or, and files are evaluated together with AND when filtering. Tags within types are also evaluated using AND.

You can't mix and match types_or and files without including the for matching types in files.

Since Rust linting rules change with new versions of cargo (usually released alongside new versions of Rust), this is needed to avoid having to completely devolve into using files instead of types.

@asottile
Copy link
Member

asottile commented Jul 7, 2024

same as #460 (comment)

but a little extra flavor on types -- they're not intended to solve all problems -- just the common easy case. when you deviate from the norm then you're going to need to pull out files (and that's ok! it should feel wrong to do so!). the way I see it is types is the golden path and what you're trying to do doesn't fit nicely so we shouldn't try and bend the system to make something work

@asottile asottile closed this Jul 7, 2024
@jalaziz
Copy link
Contributor Author

jalaziz commented Jul 8, 2024

I'd like to humbly ask that you reconsider this. How is this different than setup.cfg, pylintrc, .isort.cfg, or any of the other config file matching that is already supported by this library? Just seems a bit arbitrary to support Python and Shell tooling config files, but not Rust ones. Especially when changing the Rust version in a rust-toolchain should trigger the type of linting rules that you would expect to be run with pre-commit (much in the same way that changing .pylintrc or .isort.cfg).

it should feel wrong to do so!

Do you mean it shouldn't feel wrong? If it should feel wrong, isn't that an argument to add support for more common well-known types?

@asottile
Copy link
Member

asottile commented Jul 8, 2024

How is this different than setup.cfg, pylintrc, .isort.cfg

it's different because the file is rust-toolchain.toml and so it is already correctly identified as a toml file whereas the examples you've chosen were not previously identified as ini files

Especially when changing the Rust version in a rust-toolchain should trigger the type of linting rules that you would expect to be run with pre-commit (much in the same way that changing .pylintrc or .isort.cfg).

no it should not -- this is not how pre-commit is designed to work.

Do you mean it shouldn't feel wrong?

nope

@pre-commit pre-commit locked as off-topic and limited conversation to collaborators Jul 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants