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 buf-related files #460

Closed
wants to merge 1 commit into from
Closed

Conversation

jalaziz
Copy link
Contributor

@jalaziz jalaziz commented May 13, 2024

Tag buf.yaml, buf.gen.yaml, and buf.lock files with buf and buf-lock tags respectively.

Note: This doesn't identify custom gen files of the form buf.gen.foo.yaml as buf.

Tag buf.yaml, buf.gen.yaml, and buf.lock files with `buf` and `buf-lock`
tags respectively.

Note: This doesn't identify custom gen files of the form
buf.gen.foo.yaml as `buf`.
@jalaziz
Copy link
Contributor Author

jalaziz commented May 13, 2024

These would be pretty useful when working with protobuf files and buf.

The one issue is that you may have multiple "gen" files, with slightly different extensions. They could be easily supported by simply registering buf or buf.gen, but that seems like more of a hack than a fix. Maybe buf.gen makes sense, but not too sure about buf.

I also didn't include the buf.work.yaml workspace file since it's unclear if there's a need to perform pre-commit steps when that file changes.

More info can be found here.

@asottile
Copy link
Member

is there a reason to add these? filtering with files should work fine for these and I don't think types gets you anything here

@jalaziz
Copy link
Contributor Author

jalaziz commented May 30, 2024

Similar reason to: #459 (comment).

It's needed to avoid the limitation of having to solely use files (due to the inability to OR across both) and losing all the benefits that types has to offer.

The buf pre-commit hooks already use the proto type. Without this change, you end up having to use files exclusively to match against the buf files AND the proto files. Which also means you have to explicitly override types. All doable, just not so easily discoverable given the layers of gotchas (i.e. types vs types_or and the inability to combine files and types_or via OR).

@asottile
Copy link
Member

asottile commented Jul 7, 2024

pre-commit's design of the filters does not intend to support target files plus the config files for a hook -- it's really meant to just be the target files (and the individual who is updating the config files should be running pre-commit run --all-files themselves separately when updating! (or rely on some CI process)). including both there really doesn't work because the tool will receive the config files as positional arguments

as such I don't really see a need for this particular special identification in identify and so for now I'm going to decline this

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

jalaziz commented Jul 8, 2024

I understand your reasoning, but I think the buf pre-commit hooks is a good example. While I understand the filtering is meant to actually filter the files that are passed to the hook, it's not uncommon to use filtering as a method of detection (e.g. "only run this hook if these files have changed). In fact, all buf hooks have pass_filenames: false and use types purely as a trigger.

I do agree that's not reason enough to reconsider this.

What I do think is a reason, though, is the user experience of wanting to use a third-party official hook (e.g. buf), wanting to extend it with files and then finding out it's all broken because types and files mix in a somewhat unexpected way (until you've read the docs). After reading the docs, you then find yourself needing to lookup the source of the third-party hook, coming to this repo to determine what the correct regex is for the types used by the third-party hook, then recreate all of it in files directive. On top of all that, due to the extra matching that types can do, it may actually be impossible to recreate types as `files.

Anyway, I do understand your opinion here, but I hope you see my (the user) side of this and that the user experience around files and types with regards to extending third-party hooks is a broken one.

@asottile
Copy link
Member

asottile commented Jul 8, 2024

In fact, all buf hooks have pass_filenames: false and use types purely as a trigger.

then the buf hooks are implemented incorrectly and miss most of the point of the framework -- perhaps this is why you're having such a hard time with them.

Anyway, I do understand your opinion here, but I hope you see my (the user) side of this and that the user experience around files and types with regards to extending third-party hooks is a broken one.

it's exceedingly rare to do so, and even then it is documented with examples.

again it should feel wrong because you are probably holding it wrong if you're reaching for such a thing

@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