feat(gazelle): Add "python_default_visibility" directive #1787
Conversation
|
I think this is ready for review now. Sorry for the big PR! Most of it is just test cases though. |
| switch directiveArg := strings.TrimSpace(d.Value); directiveArg { | ||
| case "NONE": | ||
| config.SetDefaultVisibility([]string{}) | ||
| case "DEFAULT": |
There was a problem hiding this comment.
I also thought of "RESET" instead.
There was a problem hiding this comment.
I think DEFAULT is more self-descriptive.
gazelle/python/configure.go
Outdated
| // TODO: figure out how to keep this in sync with pythonconfig.go New *Config | ||
| defaultVisibilityFmtString := "//%s:__subpackages__" |
There was a problem hiding this comment.
Make this an exported const in the pythonconfig.go.
| switch directiveArg := strings.TrimSpace(d.Value); directiveArg { | ||
| case "NONE": | ||
| config.SetDefaultVisibility([]string{}) | ||
| case "DEFAULT": |
There was a problem hiding this comment.
I think DEFAULT is more self-descriptive.
gazelle/README.md
Outdated
|
|
||
| ```starlark | ||
| # an absurd example | ||
| # gazelle:python_default_visibility //$python_root:__pkg__,//foo/$python_root/tests:__subpackages__,//$python_root/$python_root/foo:bar |
There was a problem hiding this comment.
Thinking more, //$python_root/$python_root/foo:bar will probably confuse users. Let's keep this more straightforward and remove it?
There was a problem hiding this comment.
SGTM.
The purpose was to highlight that all instances of $python_root would get replaced and that it doesn't have to be at the start of the target. It's probably clearer to just say that explicitly.
| load("@rules_python//python:defs.bzl", "py_library") | ||
|
|
||
| # Reset the default visibility to the default for all child packages. | ||
| # gazelle:python_default_visibility DEFAULT |
There was a problem hiding this comment.
What if we set # gazelle:python_visibility together with # gazelle:python_default_visibility?
There was a problem hiding this comment.
How about a test case that uses # gazelle:python_visibility on a sub-package?
There was a problem hiding this comment.
test8 sets # gazelle:python_visibility on the root and then # gazelle:python_default_visibility on a sub-package.
Added the inverse of that as test9.
Co-authored-by: Thulio Ferraz Assis <[email protected]>
There was a problem hiding this comment.
LGTM, but would be great to have a CHANGELOG.md entry announcing the feature. I think pointing users to gazelle docs would be sufficient enough.
It's really nice to have extensive test coverage!
@f0rmiga, what do you think about readiness of this?
|
Thanks for addressing my comments! Thanks @aignas for the review. I'll let you merge this when the changelog looks good to you. |
…bility directive (#1835) Make the substitution pattern for `python_default_visibility` consistent with the existing `python_*_naming_convention` pattern. In #1787 I added the `python_default_visibility` directive and used a substitution pattern `$python_root`. However, I missed that the existing `python_*_naming_convention` directives include a trailing `$`. This PR is just: ``` rg -l -F "\$python_root" | xargs sed -i 's/\$python_root/$python_root$/g' ```
Fixes #1783. Provides a way to fix #1682.
Add the
python_default_visibilitydirective. This directive sets thevisibilityattribute on all generatedpy_*rules. It accepts acomma-separated list of labels to add as visibility targets, similar
to how the base
default_visibilitydirective works.Setting this directive multiple times will override previous values.
Two special values are also accepted:
NONEandDEFAULT. See./gazelle/README.md#directive-python_default_visibility for details.
The directive also accepts a special string
"$python_root"that getsreplaced with the
python_rootvalue, if set. If not set,"$python_root"is replaced with the empty string.