-
Notifications
You must be signed in to change notification settings - Fork 380
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
go_repository takes Gazelle directives #603
Conversation
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 also add something to internal/go_repository_test.go
to verify this works.
internal/go_repository.bzl
Outdated
@@ -147,7 +147,13 @@ def _go_repository_impl(ctx): | |||
print("fetch_repo: " + result.stderr) | |||
|
|||
if generate: | |||
# Build file generation is needed | |||
# Build file generation is needed. Populate Gazelle directive at root build file | |||
ctx.file( |
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.
We should only write this if ctx.attr.gazelle_directives
is set.
Ideally we would append to the file rather than replacing it if it already exists. I don't think there's a simple way to do that though. Perhaps setting this attribute should be an error if the file exists.
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.
If build_file_generation = "auto"
and build files exist in the repo, Gazelle will not run and no Gazelle directive is needed. We can just ignore them. If build_file_generation = "on"
, go_repository
will override the existing build files if there is any. Overriding the root build file with Gazelle directives feels consistent with the behavior. Do you think it necessary to report an error for this case?
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.
Thanks! Looks good, but I've pushed one small change, contradicting my earlier feedback: directives
should probably be called build_directives
instead, since there are several other attributes related to build files that start with build_
.
As discussed in #598, this PR make
go_repository
take a list of Gazelle directives to be written to the root level build file.This PR supercedes #597 and #598