feat(gazelle): Add "python_visibility" directive that appends additional visibility labels by dougthor42 · Pull Request #1784 · bazel-contrib/rules_python

dougthor42 · 2024-02-27T20:53:49Z

Add a new gazelle directive, python_visibility, that allows
users to add labels to the visibility attribute of generated targets.
This directive acts similar to¹ the go_visibility
directive.

The primary use case is for python projects that separate unit test
files from the python packages/modules that they test, like so:

packaging_tutorial/
├── LICENSE
├── pyproject.toml
├── README.md
├── src/
│   └── mypackage/
│       ├── __init__.py
│       └── foo.py
└── tests/
    ├── __init__.py
    └── test_foo.py

A future PR will add an example to the ./examples directory (issue
#1775).

At least, similar based on docs. I haven't done any actual
comparison. ↩

gazelle/python/generate.go

dougthor42 · 2024-02-27T20:58:01Z

gazelle/python/target.go

+	for _, item := range visibility {
+		t.visibility.Add(item)
+	}


I thought that I'd be able to do

t.visibility.Add(visibility...)

instead, but I would get this error:

python/target.go:104:19: cannot use visibility (variable of type []string) as type []interface{} in argument to t.visibility.Add

I'm relatively new to go and haven't use the gods.TreeSet package before, so I'm not sure why things don't work.

You can follow the pattern from above. See addModuleDependencies.

Hmm... looks like doing so would mean having to adjust the arg type from []string to *treeset.Set, which would cascade to generate.go.

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 *treeset.Set and I'll be happy to change it.

f0rmiga

Thanks for adding tests.

f0rmiga · 2024-02-28T00:37:45Z

gazelle/README.md

 | `# gazelle:resolve py ...` | n/a |
 | Instructs the plugin what target to add as a dependency to satisfy a given import statement. The syntax is `# gazelle:resolve py import-string label` where `import-string` is the symbol in the python `import` statement, and `label` is the Bazel label that Gazelle should write in `deps`. | |
+| [`# gazelle:python_visibility label`](#directive-python_visibility) | |
+| Append additional visibility labels to each generated target. This directive can be set multiple times. | |


Suggested change

| Append additional visibility labels to each generated target. This directive can be set multiple times. | |

| Appends additional visibility labels to each generated target. This directive can be set multiple times. | |

f0rmiga · 2024-02-28T00:37:59Z

gazelle/README.md

+
+#### Directive: `python_visibility`:
+
+Append additional `visibility` labels to each generated target.


Suggested change

Append additional `visibility` labels to each generated target.

Appends additional `visibility` labels to each generated target.

f0rmiga · 2024-02-28T00:40:47Z

gazelle/pythonconfig/pythonconfig.go

+	// ExtraVisibility represents the directive that controls what additional
+	// visibility labels are added to generated targets. It mimics the behavior
+	// of the `go_visibility` directive.
+	ExtraVisibility = "python_visibility"


To follow the pattern:

Suggested change

ExtraVisibility = "python_visibility"

Visibility = "python_visibility"

SGTM, updated..

f0rmiga · 2024-02-28T00:42:51Z

gazelle/python/target.go

+	for _, item := range visibility {
+		t.visibility.Add(item)
+	}


You can follow the pattern from above. See addModuleDependencies.

dougthor42

Thanks for adding tests.

Of course! I wouldn't have it any other way.

Addressed review comments, only one open question if you want me to change to addVisibility(visibility *treeset.Set)

dougthor42 · 2024-02-28T03:48:44Z

gazelle/README.md

 | `# gazelle:resolve py ...` | n/a |
 | Instructs the plugin what target to add as a dependency to satisfy a given import statement. The syntax is `# gazelle:resolve py import-string label` where `import-string` is the symbol in the python `import` statement, and `label` is the Bazel label that Gazelle should write in `deps`. | |
+| [`# gazelle:python_visibility label`](#directive-python_visibility) | |
+| Append additional visibility labels to each generated target. This directive can be set multiple times. | |


dougthor42 · 2024-02-28T03:48:49Z

gazelle/README.md

+
+#### Directive: `python_visibility`:
+
+Append additional `visibility` labels to each generated target.


dougthor42 · 2024-02-28T03:54:00Z

gazelle/python/target.go

+	for _, item := range visibility {
+		t.visibility.Add(item)
+	}


Hmm... looks like doing so would mean having to adjust the arg type from []string to *treeset.Set, which would cascade to generate.go.

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 *treeset.Set and I'll be happy to change it.

dougthor42 · 2024-02-28T03:54:13Z

gazelle/pythonconfig/pythonconfig.go

+	// ExtraVisibility represents the directive that controls what additional
+	// visibility labels are added to generated targets. It mimics the behavior
+	// of the `go_visibility` directive.
+	ExtraVisibility = "python_visibility"


SGTM, updated..

aignas · 2024-02-28T13:53:03Z

Note, that the PR will get squashed and the commit message on the main branch will be the description of the PR. So consider improving the wording/layout there.

That said, I like PRs being submitted as a way to spark the discussion, it's not rude at all. It's sometimes really nice to discuss something concrete. So thank you for doing just that!

dougthor42 · 2024-02-28T15:57:44Z

PR message updated - removed the note and kept lines down to ~72 chars.

I like PRs being submitted as a way to spark the discussion, it's not rude at all. It's sometimes really nice to discuss something concrete. So thank you for doing just that!

You're welcome! Some people like it, others hate it haha. Personally I also find it nice to discuss something concrete.

f0rmiga

Looks good, just address my last comments and ping me to merge. Thanks!

f0rmiga · 2024-03-01T01:02:57Z

gazelle/python/generate.go


 	parser := newPython3Parser(args.Config.RepoRoot, args.Rel, cfg.IgnoresDependency)
-	visibility := fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)
+	// TODO(gh-1682): Add support for default_visibility directive and replace


Please, remove this TODO item since there's an issue already.

f0rmiga · 2024-03-01T01:06:36Z

gazelle/pythonconfig/pythonconfig.go

 	libraryNamingConvention      string
 	binaryNamingConvention       string
 	testNamingConvention         string
+	extraVisibility              []string


I hoped you'd remove the extra everywhere.

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 👍.

aignas · 2024-03-13T01:04:03Z

I see that the CHANGELOG.md did not get updated when merging this PR. Would one of you be willing to create a followup PR to add the a short snippet about the new features? Something like:

Added:
* (gazelle) Added a new `python_visibility` directive to control visibility of
  generated targets to allow sharing of targets between trees with different
  `python_root` values.

I am not sure if I worded it well enough, so feel free to improve upon in!

dougthor42 · 2024-03-13T01:14:14Z

Ah sorry. I'll do that tonight. I thought it was automatic as part of the Conventional Commits.

…

On Tue, Mar 12, 2024, 18:04 Ignas Anikevicius ***@***.***> wrote: I see that the CHANGELOG.md did not get updated when merging this PR. Would one of you be willing to create a followup PR to add the a short snippet about the new features? Something like: Added: * (gazelle) Added a new `python_visibility` directive to control visibility of generated targets to allow sharing of targets between trees with different `python_root` values. I am not sure if I worded it well enough, so feel free to improve upon in! — Reply to this email directly, view it on GitHub <#1784 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJDFEJYNHQOWDXISSUZEWLYX6QZVAVCNFSM6AAAAABD42DP26VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJSHEYDSMJXGE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

Add the missing changelog from PR #1784.

dougthor42 added 3 commits February 27, 2024 12:10

Add test case

7ee8023

Add python_visibility directive

8c4680d

Update README

4fe0e69

dougthor42 commented Feb 27, 2024

View reviewed changes

spelling

6127e39

dougthor42 marked this pull request as ready for review February 27, 2024 22:22

dougthor42 requested a review from f0rmiga as a code owner February 27, 2024 22:22

f0rmiga suggested changes Feb 28, 2024

View reviewed changes

dougthor42 added 4 commits February 27, 2024 19:37

Merge branch 'main' into gazelle-python-visibility

e084263

Move things around in README.md

afcec2b

s/append/appends/g in readme per code review.

e3ec878

s/Extra//g per code review

ccb9b49

dougthor42 commented Feb 28, 2024

View reviewed changes

dougthor42 requested a review from f0rmiga February 29, 2024 21:39

f0rmiga approved these changes Mar 1, 2024

View reviewed changes

dougthor42 added 2 commits February 29, 2024 20:21

Remove TODO

dbe4724

_actually_ remove all the 'extra' prefixes.

c48a65b

dougthor42 requested a review from f0rmiga March 1, 2024 04:28

f0rmiga added this pull request to the merge queue Mar 1, 2024

Merged via the queue into bazel-contrib:main with commit da10ac4 Mar 1, 2024

aignas mentioned this pull request Mar 1, 2024

fix(gazelle): _actually_ don't write narrow dependencies. #1683

Closed

dougthor42 mentioned this pull request Mar 1, 2024

Add support for gazelle's default_visibility directive #1786

Closed

dougthor42 deleted the gazelle-python-visibility branch March 1, 2024 21:03

dougthor42 added a commit to dougthor42/rules_python that referenced this pull request Mar 13, 2024

Update changelog for PR bazel-contrib#1784

2e8b1c0

dougthor42 mentioned this pull request Mar 13, 2024

docs(changelog) Update changelog from PR #1784 #1802

Merged

github-merge-queue bot pushed a commit that referenced this pull request Mar 13, 2024

docs(changelog) Update changelog from PR #1784 (#1802)

9c538ea

Add the missing changelog from PR #1784.

dougthor42 mentioned this pull request May 30, 2024

Allow $python_root$ in the # gazelle:python_visibility directive #1932

Closed

	\| Append additional visibility labels to each generated target. This directive can be set multiple times. \| \|
	\| Appends additional visibility labels to each generated target. This directive can be set multiple times. \| \|


		#### Directive: `python_visibility`:

		Append additional `visibility` labels to each generated target.

	Append additional `visibility` labels to each generated target.
	Appends additional `visibility` labels to each generated target.

	ExtraVisibility = "python_visibility"
	Visibility = "python_visibility"

Uh oh!

Conversation

dougthor42 commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f0rmiga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougthor42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aignas commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougthor42 commented Feb 28, 2024

Uh oh!

f0rmiga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aignas commented Mar 13, 2024

Uh oh!

dougthor42 commented Mar 13, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dougthor42 commented Feb 27, 2024 •

edited

Loading

aignas commented Feb 28, 2024 •

edited

Loading