Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(gazelle): Add "python_test_file_pattern" directive #1819
feat(gazelle): Add "python_test_file_pattern" directive #1819
Changes from 20 commits
056c4e4
ac364d2
a852679
d4b456b
5cbcd67
abe667c
98922c2
f9897b0
a3cc524
cf274c3
a156e2e
f1b66a2
9eb66f4
0f2b9aa
7b7648b
fd21bc9
ca79dfa
17e14d7
19b9a49
5875c2f
df8f967
b14b0dd
d3a5364
988ae8c
4d41e94
58d5571
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there is no need to double severity in the message here as
Fatal
will be already in the log. Also, go's error messages are conventionally lowercased since errors can be wrapped/nested (e.g.something bad happened: directive 'python...
).\n
is also unnecessary.(See https://go.dev/wiki/CodeReviewComments#error-strings)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, thanks! Updated.
As an FYI, the
ERROR:
and\n
were included to maintain consistency with the otherlog.Fatal
messages:Prior to this PR, 7 of 9 calls included both
ERROR:
and\n
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am slightly concerned that we are compiling the glob here. Would it be possible to compile it elsewhere? When parsing the config would be another option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is used in larger repos I am worried that we perform a linear number of glob compilations as opposed to doing it once when loading the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We were doing that before1 with
gobwas/glob
. From what I can tell,doublestar
doesn't have any kind of "compiled pattern" object that I can pass around instead.We can validate patterns during config load, but we wouldn't be able to tell
doublestart.Match
to not validate patterns and thus save some cycles. (Unless there's a way to accessmatchWithSeparator
? My understanding of go is that lowercase names aren't exported and are thus not accessible.)I think we have two (three?) options:
gobwas/glob
and precompile patternsEither way we should validate/compile patterns when loading the config so that we fail earlier. I've made that update. It means we validate one more time, but we're already validating (N_files * Patterns) times so (N*P)+1 is no big deal
Footnotes
Well, almost. It wasn't done when loading config, but at least it was outside of the
for _, f := range args.RegularFiles
loop. ↩There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the match's code there is nothing to "compile" in glob really, like we typically do with regex. This is common for globs to be matched directly as they are simple, do not need complicated parsing, creating underlying tree used for matching etc.
This [rules_python] repo already does
doublestart.Match
in a loop here and so does bazel-gazelle here and it hasn't been an issue so far.Validating earlier is a good improvement.
That is correct for go. I created a ticket bmatcuk/doublestar#92