Skip to content

Update bazel to match only extension or whole name#1

Merged
jlebar merged 3 commits intojlebar:masterfrom
TheButlah:master
Apr 11, 2020
Merged

Update bazel to match only extension or whole name#1
jlebar merged 3 commits intojlebar:masterfrom
TheButlah:master

Conversation

@TheButlah
Copy link
Contributor

@TheButlah TheButlah commented Apr 11, 2020

The hook was originally matching all files in the repository as long as they had BUILD or BUILD.bazel in them. This commit ensures that, for example, BUILD.bazel and whatever/BUILD.bazel match, but not whateverBUILD.bazelasdf. It also adds support for whatever.BUILD files.

The hook was originally matching all files in the repository as long as they had `BUILD` or `BUILD.bazel` in them. This meant that they actually overrode the `files` setting in the user's repository. This commit ensures that, for example, `BUILD.bazel` and `whatever/BUILD.bazel` match, but not `whateverBUILD.bazelasdf`. It also adds support for `whatever.BUILD` files.
@TheButlah
Copy link
Contributor Author

TheButlah commented Apr 11, 2020

Duplicated from FelixSeptem/pre-commit-golang#3

@jlebar
Copy link
Owner

jlebar commented Apr 11, 2020

Thank you so much for the PR!

I agree the original code is broken (oops), but I'm not 100% sure the new code works as intended either. I may just be misunderstanding, though.

(^(BUILD\.bazel|BUILD)|/\2|\.BUILD)$

Deconstructed this is:

(
  ^(BUILD\.bazel|BUILD) |
  /\2 |
  \.BUILD
)$

I've only really used RE2, which doesn't support backreferences, so I may be misunderstanding how this is supposed to work. But AIUI /\2 will only match something if group 2 matches something first, but since there's an alternation (i.e. |), we can never match both group 2 and /\2.

WDYT, am I missing something?

@TheButlah
Copy link
Contributor Author

Yep, I think you're right. Here's an updated regex: https://regex101.com/r/AYsCnu/1/
I can update the PR with the new version tomorrow

@jlebar
Copy link
Owner

jlebar commented Apr 11, 2020

I was confused what (|foo) was supposed to do, but now I realize it's the same as (foo)?.

WDYT of the following; maybe it's slightly easier to parse?

(?:^|.*/)(?:BUILD\.bazel|BUILD)$|\.BUILD$

https://regex101.com/r/f0fFhE/1

@TheButlah
Copy link
Contributor Author

TheButlah commented Apr 11, 2020

I agree, using (whatever)? instead of (|whatever) is better. That being said, why the non-capturing groups? pre-commit doesn't care about what the groups are, only whether a match is found so making them non-capturing just adds extra complexity to the expression.

How about this as the simplest version: https://regex101.com/r/f0fFhE/2

^(.*/)?(BUILD\.bazel|BUILD)$|\.BUILD$

@jlebar
Copy link
Owner

jlebar commented Apr 11, 2020

why the non-capturing groups?

Eh, you're right, it was a premature optimization. The whole purpose of noncapturing groups is to let the regex engine optimize things that it otherwise couldn't, but it doesn't matter in this case.

@jlebar
Copy link
Owner

jlebar commented Apr 11, 2020

LGTM, merging!

Thanks again for the PR.

@jlebar jlebar merged commit 1a9e22b into jlebar:master Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants