I might prefer a more helpful error message if the glob fails. Could we add an integration test? Maybe it is good enough already? Right now this code would panic

• Failure to compile: error message added, test added

I had a similar thought last night: we also need to handle the case where the user has # gazelle:python_test_file_pattern (with no value). Right now no files will be mapped to py_test if there is no pattern. If we want to keep that behavior, I'll need to update the docs.

• Failure to compile: error message added, test added

I had a similar thought last night: we also need to handle the case where the user has # gazelle:python_test_file_pattern (with no value). Right now no files will be mapped to py_test if there is no pattern. If we want to keep that behavior, I'll need to update the docs.

In #1787 I added an option for resetting the directive to the rules_python default. Do we want to support the same for this directive?

// N.B.: pseudocode; haven't tested yet
case pythonconfig.TestFilePattern:
    directiveArg := strings.TrimSpace(d.Value)
    if directiveArg == "DEFAULT" {
        directiveArg = pythonconfig.DefaultTestFilePatternString
    }
    globStrings := strings.Split(directiveArg, ",")
    config.SetTestFilePattern(globStrings)

For consistency I would agree, but at the same time my brain is also telling me that we could have:

gazelle:python_test_file_pattern

to reset the config to defaults.

The empty pattern yielding no test targets is not super intuitive, IMHO.

What do you think?

The empty pattern yielding no test targets is not super intuitive, IMHO.

Agreed.

That said, I would argue that the empty pattern resetting to defaults is also not super intuitive. Some readers will see that and think "no test targets" while others will think "resets to default".

I like explicit things 😁. There's no ambiguity and it's easier to grok when returning to something N years down the road.

If we wanted to support yielding no test targets¹ it should be something like gazelle:python_test_file_pattern NONE or have a different directive altogether (gazelle:python_no_tests or something).

Passerby opinion: yielding no test targets can (and imho should) be achieved by ignoring files with gazelle:python_ignore_files. That said, setting pattern to NONE should not be allowed (also, it's a bit confusing to set pattern to NONE, some might thing there is NONE file or smths). gazelle:python_test_file_pattern (blank) should also fail.

yielding no test targets can (and imho should) be achieved by ignoring files with gazelle:python_ignore_files

I disagree. There are definitely cases where you want to yield no test targets but do want to yield lib and binary targets.

project/
+ BUILD.bazel
+ MODULE.bazel
+ src/myfoo/
    + BUILD.bazel
    + foo.py
    + foo_test.py
    + subdir/
        + BUILD.bazel
        + run_test.py      # py_binary
        + example_test.py  # py_library

Using gazelle:python_ignore_files would mean that no targets are generated in subdir above, when really we'd want only py_library and py_binary targets made.

There's also the case where test files are outside of the package, as described by PyPA Tutorial. In that case, you can be sure that all targets in src/mypackage below are py_library and py_binary.

project/
+ BUILD.bazel
+ MODULE.bazel
+ src/mypackage/               # guaranteed to have no py_test targets
    + BUILD.bazel
    + run_electrical_test.py   # py_binary
    + electrical_test.py       # py_library
+ tests/                       # any py_* targets
    + BUILD.bazel
    + foo_test.py              # py_test
    + bar_test.py              # py_test
    + test_utils.py            # py_library
    + run_all_test.py          # py_binary

also, it's a bit confusing to set pattern to NONE, some might thing there is NONE file

A valid point. Perhaps using something that is unlikely to be a filename would be better? Maybe one of these or similar?

# gazelle:python_test_file_pattern %NONE%
# gazelle:python_test_file_pattern !!NO_FILES
# gazelle:python_test_file_pattern ~!~NO_PATTERN~!~

One issue is that linux filenames can be basically anything haha so it's not feasible to pick a string that can't be a filename.

If we do go with a more "special" string, I'd probably want to update the python_default_visibility directive special strings to match.

gazelle:python_test_file_pattern (blank) should also fail.

+1 to that.

Agreed with arguments around gazelle:python_ignore_files.

How about...not having any magic names at all? I mean, by the time someone wants to reset the test file patterns, he/she already learned what default values are (while customising them in the first place), and if resetting the patterns is needed, it can be done with # gazelle:python_test_file_pattern *_test,test_*.

Yet another idea is to have dedicated no-args directive for resetting to default, something like:
# gazelle:default_python_test_file_pattern. However, it only makes sense if applied conventionally everywhere in the plugin imho. Otherwise, I'd vote for not having any special values and let users "reset" by explicitly setting python_test_file_pattern to default values.

I'm good with no magic names at all and a "reset" by explicitly setting the value to defaults. gazelle:python_test_file_pattern *_test.py,test_*.py

That said, I'd still like to mention the two (minor) issues with no magic values:

1. [can ignore] There's no way to defer to rules_python's defaults. For example, if rules_python changes the default from *_test.py,test_*.py to something else, users would have to update their configs manually if they wanted to always use the rules_python default.
   • TBH this is probably a good thing because it results in less unexpected behavior.
2. We're back to the # gazelle:python_test_file_pattern (no value) for preventing any files from mapping to py_test. As mentioned previously:
   • this is not very intuitive.
   • we said that "no value" should fail.

For (2), perhaps just making sure that's documented and tested is sufficient? Unless we implement another directive gazelle:python_no_test_files or something (which I'm against - it seems to only complicate things).

have dedicated no-args directive for resetting to default ... it only makes sense if applied conventionally everywhere in the plugin

+1. I'm slightly unsure about having directives to reset to default (as you said, people can just set the default manually), but if we add them they definitely should be applied everywhere.

As far as I can see, the plugin currently has no switches for enabling/disabling parts of the generation process. Setting pattern to none sounds to me like an abuse of the directive to disable tests and I'd rather have an explicit python_no_test_files so that there is a clear indication no tests are being processed for a) confusing fewer humans, and b) plugin itself, so that it can potentially deviate its processing depending on that fact (e.g. print debug/warning message etc).

This should be, however, out of context for this PR. I'd say, we should just disallow no-arg python_test_file_pattern, provide no magic values and wrap-up the PR.

Have I already mentioned this is great work and nice improvement 🙂? Thank you! Looking forward to it. I have exactly the place where I will use it already.

Setting pattern to none sounds to me like an abuse of the directive

I can see that and it's a good point.

I'd rather have an explicit python_no_test_files so that there is a clear indication no tests are being processed ... we should just disallow no-arg python_test_file_pattern, provide no magic values and wrap-up the PR.

SGTM. I'd like to continue the discussion, so I've opened #1826.

this is great work and nice improvement

Thanks! I've been really enjoying being able to add more features.

All this SGTM, thanks for the discussion. I've been enjoying the read! :)

This seems to be the most popular glob package, though I don't see any github activity in 6 years. Other options were:

• https://pkg.go.dev/github.com/ryanuber/go-glob
• https://pkg.go.dev/v.io/v23/glob

How about doublestar.Match? It is used by bazel-gazelle and is already included in this go.mod:9 too.

import (
	"fmt"

	"github.com/bmatcuk/doublestar/v4"
)

func matchesAnyGlob(s string, globs []string) bool {
	for _, g := range globs {
		if ok, _ := doublestar.Match(g, s); ok {
			return true
		}
	}
	return false
}

func main() {
	defaultGlob := []string{"*_test.py", "test_*.py"}
	fmt.Println(matchesAnyGlob("test_my_target.py", defaultGlob))
	fmt.Println(matchesAnyGlob("my_target_test.py", defaultGlob))

	customGlob := []string{"*_pytest.py", "pytest_*.py"}
	fmt.Println(matchesAnyGlob("pytest_upload.py", customGlob))
	fmt.Println(matchesAnyGlob("download_pytest.py", customGlob))
}

+1 for not adding another dep! Updated.

+1 for not adding another dep! Updated.

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.

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.

Agreed. We were doing that before¹ 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 access matchWithSeparator? My understanding of go is that lowercase names aren't exported and are thus not accessible.)

I think we have two (three?) options:

1. Go back to gobwas/glob and precompile patterns
2. Accept the perf hit.
3. Some go ability that I'm not aware of?

Either 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

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.

Unless there's a way to access matchWithSeparator? My understanding of go is that lowercase names aren't exported and are thus not accessible.

That is correct for go. I created a ticket bmatcuk/doublestar#92

Agreed. We were doing that before¹ 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 access matchWithSeparator? My understanding of go is that lowercase names aren't exported and are thus not accessible.)

I think we have two (three?) options:

1. Go back to gobwas/glob and precompile patterns
2. Accept the perf hit.
3. Some go ability that I'm not aware of?

Either 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

globstar errors with path.ErrBadPAttern but also states "users should not count on the returned error, doublestar.ErrBadPattern, being equal to path.ErrBadPattern" so I've opted to inject the string manually.

Actually... I could just do:

// We only care if the pattern is valid. Use `Match` instead of `ValidatePattern` (which
// just returns `bool`) so that we always use the error message provided by `doublestar`.
if _, err := doublestar.Match(p, ""); err != nil {
    log.Fatalf("ERROR: Failed to compile glob '%v'. Error: %v\n", p, err)
}
...

Any preference between
(a) using ValidatePattern and manually injecting the error string as I've already done or
(b) abusing Match?

I guess the word "abusing" in your comment speaks for itself :) Looking at the Match's code, err.ErrBadPattern brings no more than just an error flag (no error message, no extra context etc.) so it is effectively does the same as having true/false. I'd keep it as is. Except maybe a nit regarding go error message style (see the comment in configure.go).

👍 kept, aside from style nit

Would this check be better in configure.go case pythonconfig.TestFilePattern:?

I'd say it would be slightly more aligned with existing code to do to in a case indeed as there are already some invalid value errors and parsing there while these stay simple setters.

👍 moved.

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)

TIL, thanks! Updated.

As an FYI, the ERROR: and \n were included to maintain consistency with the other log.Fatal messages:

$ rg 'log.Fatal.?\("'
manifest/generate/generate.go
90:             log.Fatalf("ERROR: %v\n", err)
109:            log.Fatalf("ERROR: %v\n", err)

pythonconfig/pythonconfig.go
444:                    log.Fatalf("ERROR: Failed to compile glob '%v'. Error: syntax error in pattern\n", p)

python/lifecycle.go
41:                     log.Fatalf("failed to write parser zip: %v", err)
47:                     log.Fatalf("cannot write %q: %v", helperPath, err)

python/generate.go
238:                    log.Fatalf("ERROR: %v\n", err)
319:                    log.Fatalf("ERROR: %v\n", err)
353:                    log.Fatalf("ERROR: %v\n", err)
384:                    log.Fatalf("ERROR: %v\n", err)
504:            log.Fatalf("ERROR: %v\n", err)

python/configure.go
188:                            log.Fatal("ERROR: Directive 'python_test_file_pattern' requires a value.")

Prior to this PR, 7 of 9 calls included both ERROR: and \n.