Skip to content
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

Forward kwargs to _license #8

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Forward kwargs to _license #8

merged 2 commits into from
Feb 1, 2022

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Oct 30, 2021

This makes it possible to e.g. set visibility on license targets.

This makes it possible to e.g. set visibility on license targets.
@aiuto
Copy link
Collaborator

aiuto commented Jan 20, 2022

Sorry I missed this before.

So, in general I think kwargs should pass through, but your PR comment intrigued me.
Why would you want any visibility other than public? You can't actually do enforcement if the visibility fails. It's not actually clear visibility would matter for the aspect traversal anyway.

Perhaps we only need to pass through visibility? Or maybe just force visibility=public.

@fmeum
Copy link
Contributor Author

fmeum commented Jan 20, 2022

I didn't give this much thought, I just noticed that there wasn't any way to set a visibility other than private. Forcing visibility to be public sounds right to me based on your argument.

@aiuto
Copy link
Collaborator

aiuto commented Feb 1, 2022

Sorry I missed this
Let's change it to visibilty is public, but keep your fixes that use pop.
I'll try to merge that way.

Forcing license declarations to be public visible. I have yet to see a scenario where it should be private.
@aiuto aiuto merged commit f40d528 into bazelbuild:main Feb 1, 2022
@fmeum fmeum deleted the patch-3 branch February 1, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants