-
-
Notifications
You must be signed in to change notification settings - Fork 615
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
Refactor find_valid_field_sets. #10732
Conversation
- Now it's possible to get the FieldSets that subclass some given superclass for any arbitrary set of targets. - The previous logic, that applies this to the target roots, now uses this new logic. - Also removes FieldSetWithOrigin, which was unused, and would have complicated the refactoring. - Also improves error messages in setup_py.py, that are unrelated to this change but didn't merit their own change. [ci skip-rust] [ci skip-build-wheels]
f"Exporting owners for {target.address} are " | ||
f"ambiguous. Found {exported_ancestor.address} and " | ||
f"{len(sibling_owners)} others: " | ||
f"python_distribution target owning {target.address} is ambiguous. " |
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 don't technically know it's a python_distribution
. This would be misleading with subclasses. You could call target.alias
instead.
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.
target.alias
is the alias of the target whose owner we're seeking, which is typically a python_library
. The owner is a python_distribution
(or, technically, a subclass, true, but I would rather sacrifice a tiny bit of accuracy in the general case for clarity in the usual 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.
Can we call exported_ancestor.alias
?
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.
Maybe, but what if the siblings aren't of this type? And anyway, this is a pretty rare corner case. Doesn't seem worth the worry.
[ci skip-rust] [ci skip-build-wheels]
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!
|
||
def __init__(self, msg: str): | ||
super().__init__( | ||
f"{msg} See https://www.pantsbuild.org/v2.0/docs/python-setup-py-goal for " |
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.
This is likely to become stale. But it's fine given the nature of 2.0.
We'll want to audit for the string "v2.0" as soon as we switch the default version of the docs.
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.
Yeah, there is some general tooling we'll need to build around the docs (e.g., finding broken links). Basic stuff that readme.com doesn't provide yet.
given superclass for any arbitrary set of targets.
target roots, now uses this new logic.
and would have complicated the refactoring.
are unrelated to this change but didn't merit their
own change.
[ci skip-rust]
[ci skip-build-wheels]