-
Notifications
You must be signed in to change notification settings - Fork 541
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_visibility" directive that appends additional visibility labels #1784
Changes from 8 commits
7ee8023
8c4680d
4fe0e69
6127e39
e084263
afcec2b
e3ec878
ccb9b49
dbe4724
c48a65b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,9 +99,11 @@ func (t *targetBuilder) addResolvedDependency(dep string) *targetBuilder { | |
return t | ||
} | ||
|
||
// addVisibility adds a visibility to the target. | ||
func (t *targetBuilder) addVisibility(visibility string) *targetBuilder { | ||
t.visibility.Add(visibility) | ||
// addVisibility adds visibility labels to the target. | ||
func (t *targetBuilder) addVisibility(visibility []string) *targetBuilder { | ||
for _, item := range visibility { | ||
t.visibility.Add(item) | ||
} | ||
Comment on lines
+104
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that I'd be able to do t.visibility.Add(visibility...) instead, but I would get this error:
I'm relatively new to go and haven't use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can follow the pattern from above. See There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... looks like doing so would mean having to adjust the arg type from As-is, the diff is smaller and I'm always a fan of using built-in types when possible. That said, LMK if you feel strongly that this func should be |
||
return t | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Directives can be added in any order. They will be ordered alphabetically | ||
# when added. | ||
# gazelle:python_visibility //tests:__pkg__ | ||
# gazelle:python_visibility //bar:baz |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
load("@rules_python//python:defs.bzl", "py_library") | ||
|
||
# Directives can be added in any order. They will be ordered alphabetically | ||
# when added. | ||
# gazelle:python_visibility //tests:__pkg__ | ||
# gazelle:python_visibility //bar:baz | ||
|
||
py_library( | ||
name = "directive_python_visibility", | ||
srcs = ["foo.py"], | ||
visibility = [ | ||
"//:__subpackages__", | ||
"//bar:baz", | ||
"//tests:__pkg__", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Directive: `python_visibility` | ||
|
||
This test case asserts that the `# gazelle:python_visibility` directive correctly | ||
appends multiple labels to the target's `visibility` parameter. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
# This is a Bazel workspace for the Gazelle test data. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
def func(): | ||
print("library_func") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# python_visibilty directive applies to all child bazel packages. | ||
# Thus, the generated file for this package will also have vis for | ||
# //tests:__pkg__ and //bar:baz in addition to the default. | ||
# gazelle:python_visibility //tests:__subpackages__ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
load("@rules_python//python:defs.bzl", "py_library") | ||
|
||
# python_visibilty directive applies to all child bazel packages. | ||
# Thus, the generated file for this package will also have vis for | ||
# //tests:__pkg__ and //bar:baz in addition to the default. | ||
# gazelle:python_visibility //tests:__subpackages__ | ||
|
||
py_library( | ||
name = "subdir", | ||
srcs = [ | ||
"__init__.py", | ||
"bar.py", | ||
], | ||
visibility = [ | ||
"//:__subpackages__", | ||
"//bar:baz", | ||
"//tests:__pkg__", | ||
"//tests:__subpackages__", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# Copyright 2023 The Bazel Authors. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
--- | ||
expect: | ||
exit_code: 0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,10 @@ const ( | |
// naming convention. See python_library_naming_convention for more info on | ||
// the package name interpolation. | ||
TestNamingConvention = "python_test_naming_convention" | ||
// Visibility represents the directive that controls what additional | ||
// visibility labels are added to generated targets. It mimics the behavior | ||
// of the `go_visibility` directive. | ||
Visibility = "python_visibility" | ||
) | ||
|
||
// GenerationModeType represents one of the generation modes for the Python | ||
|
@@ -136,6 +140,7 @@ type Config struct { | |
libraryNamingConvention string | ||
binaryNamingConvention string | ||
testNamingConvention string | ||
extraVisibility []string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hoped you'd remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could have sworn I did... Sorry about that, I definitely meant to. ... Ah, looks like I stashed the changes instead of committing them. /facepalm. Anyway, updated 👍. |
||
} | ||
|
||
// New creates a new Config. | ||
|
@@ -157,6 +162,7 @@ func New( | |
libraryNamingConvention: packageNameNamingConventionSubstitution, | ||
binaryNamingConvention: fmt.Sprintf("%s_bin", packageNameNamingConventionSubstitution), | ||
testNamingConvention: fmt.Sprintf("%s_test", packageNameNamingConventionSubstitution), | ||
extraVisibility: []string{}, | ||
} | ||
} | ||
|
||
|
@@ -183,6 +189,7 @@ func (c *Config) NewChild() *Config { | |
libraryNamingConvention: c.libraryNamingConvention, | ||
binaryNamingConvention: c.binaryNamingConvention, | ||
testNamingConvention: c.testNamingConvention, | ||
extraVisibility: c.extraVisibility, | ||
} | ||
} | ||
|
||
|
@@ -388,3 +395,13 @@ func (c *Config) SetTestNamingConvention(testNamingConvention string) { | |
func (c *Config) RenderTestName(packageName string) string { | ||
return strings.ReplaceAll(c.testNamingConvention, packageNameNamingConventionSubstitution, packageName) | ||
} | ||
|
||
// AppendVisibility adds additional items to the target's visibility. | ||
func (c *Config) AppendVisibility(visibility string) { | ||
c.extraVisibility = append(c.extraVisibility, visibility) | ||
} | ||
|
||
// Visibility returns the target's visibility. | ||
func (c *Config) Visibility() []string { | ||
return c.extraVisibility | ||
} |
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.
Please, remove this TODO item since there's an issue already.
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.
Done!