-
-
Notifications
You must be signed in to change notification settings - Fork 652
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: add go_module and go_external_module targets #12377
Conversation
[ci skip-rust] [ci skip-build-wheels]
I tried the above diff to add a |
[ci skip-rust] [ci skip-build-wheels]
I switched to requesting |
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!
for tgt in candidate_targets | ||
if tgt.has_field(GoModuleSources) and not tgt.address.is_file_target | ||
] | ||
sorted_go_module_targets = sorted(go_module_targets, key=lambda x: x[0], reverse=True) |
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.
Simpler is probably to not store a tuple in the above collection, and sort by lambda tgt: tgt.address.spec_path
. That should simplify a couple lines in this rule
) | ||
|
||
|
||
# `go_binary` target |
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.
Consider using this style:
pants/src/python/pants/engine/internals/graph.py
Lines 89 to 91 in e01e353
# ----------------------------------------------------------------------------------------------- | |
# Address -> Target(s) | |
# ----------------------------------------------------------------------------------------------- |
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.
There are no rules in this file. This comment style doesn't make sense for just the target type definitions. It would make sense for the files with rules in them.
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 use it for python/target_types.py
too:
pants/src/python/pants/backend/python/target_types.py
Lines 85 to 87 in e392457
# ----------------------------------------------------------------------------------------------- | |
# `pex_binary` target | |
# ----------------------------------------------------------------------------------------------- |
The general idea is to have section headers that more clearly delineate the file, sort of like chapters in a book.
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.
Ah, I had thought you were referring to the Foo -> Bar
notation and not the dashes. Yeah the dashes would be relevant to the comments here.
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
go_module
target which represents a first-party Go module.go_external_module
target which is a third-party Go module.go_package
and the nearest sibling or ancestorgo_module
.Note: This PR does not include a dependency inference rule to infer the external modules listed in a
go.mod
file as dependencies of the applicablego_module
. That will be done in a follow-up.[ci skip-rust]
[ci skip-build-wheels]