feat: fix-visibility #34
Conversation
f0rmiga
force-pushed
the
fix-visibility
branch
2 times, most recently
from
908dc2e
to
3667c5b
Compare
Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>
f0rmiga
force-pushed
the
fix-visibility
branch
from
3667c5b
to
1426588
Compare
| } | ||
| matches := visibilityIssueRegex.FindSubmatch(bazelEvent.Value) | ||
| if len(matches) != 3 { | ||
| return nil |
There was a problem hiding this comment.
should it have some error handling here? or maybe this is a good case for a logger that writes some log file for us to diagnose surprising issues
There was a problem hiding this comment.
No, I'm doing regex group matching here. Since I have 2 groups, I expect 3 matches (the first being the full string). If there were no matches, we only return nil for the build event message, no errors were involved here.
| ctx, cancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| // TODO(f0rmiga): check if buildozer is installed, otherwise return an error. |
There was a problem hiding this comment.
why wouldn't we just link buildozer into our binary?
There was a problem hiding this comment.
Yeah, reading again https://github.com/bazelbuild/buildtools/blob/master/buildozer/main.go, we can probably use it directly.
There was a problem hiding this comment.
I'll do this in a follow-up PR.
| return nil | ||
| } | ||
|
|
||
| func (plugin *FixVisibilityPlugin) hasPrivateVisibility(ctx context.Context, toFix string) (bool, error) { |
There was a problem hiding this comment.
it might also not be visible because it has some non-private visibility that doesn't include the referrer, do you handle that case?
There was a problem hiding this comment.
Yeah, this is only a check for when it does have the private visibility, in which case, we need to remove it.
| type buildozer struct{} | ||
|
|
||
| func (b *buildozer) Run(ctx context.Context, args ...string) ([]byte, error) { | ||
| cmd := exec.CommandContext(ctx, "buildozer", args...) |
There was a problem hiding this comment.
yeah I'd love to just link buildozer in :)
No description provided.