pre-commit / identify

File identification library for Python
MIT License
255 stars 142 forks source link

Identify rust-toolchain files as rust-toolchain #459

Closed jalaziz closed 4 months ago

jalaziz commented 6 months ago

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 commented 5 months ago

same feedback as your other PR: https://github.com/pre-commit/identify/pull/460#issuecomment-2137525332

jalaziz commented 5 months ago

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 commented 4 months ago

same as https://github.com/pre-commit/identify/pull/460#issuecomment-2212620276

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

jalaziz commented 4 months ago

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 commented 4 months ago

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