Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

restrict visibility on filegroup target only consumed in 1 place #605

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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 2, 2022

Conversation

thundergolfer
Copy link

@thundergolfer thundergolfer commented Jan 19, 2022

Reviewd #601 after it merged. Thanks for fixing this long outstanding annoyance 馃檪 @UebelAndre

If I understand this code correctly the filegroup is only consumed by //python/pip_install/private:srcs_module and so you don't need public visibility on the target.

@thundergolfer thundergolfer requested review from hrfuller and removed request for brandjon and lberki January 19, 2022 03:01
@UebelAndre
Copy link
Contributor

Thanks for the extra review and sorry about the miss. I'll be sure to pay more attention to visibility going forward 馃榾

@thundergolfer
Copy link
Author

thundergolfer commented Jan 19, 2022

@UebelAndre thanks again for the PR, and don't worry about it. It's such a minor thing 馃檪

@thundergolfer thundergolfer requested a review from f0rmiga January 25, 2022 05:26
Copy link
Member

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's only used by //python/pip_install/private:srcs_module, as stated in the PR description, we could be even more diligent.

I'll leave it up to you to take the suggestion or not.

python/pip_install/BUILD Outdated Show resolved Hide resolved
Jonathon Belotti and others added 2 commits February 2, 2022 14:11
Co-authored-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>
@thundergolfer thundergolfer merged commit 8cf1220 into main Feb 2, 2022
@thundergolfer thundergolfer deleted the jonathon--pr601-patch branch June 14, 2022 10:58
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.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.