-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Tweak DescendantAddresses
so that call sites can no-op when no matches
#10012
Tweak DescendantAddresses
so that call sites can no-op when no matches
#10012
Conversation
# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]
I can't see why erroring for any glob is good behavior. How about fixing |
I think the idea is that you are being specific enough that we want to yell loudly when it doesn't resolve. For example, when you put I'm happy to fix, but are you sure we want to? I bias towards keeping the error, but I do not have a strong opinion here. |
OK, how about side-stepping taste / precedent altogether. Presumably the motivating code uses DescendantAddress directly via Get? If so can the appropriate rule be parameterized to allow the desired behavior as determined by the call site? Not dissimilar to path glob handling. |
Sure. To confirm, this means that the CLI will behave the same before that it will error, but the rule can now no-op? |
Yes. I still think having globs not fail when used on the CLI is better behvior (for some goals anyhow, like list) - but changing behavior of all goals to support an internal use case had a smell. |
Okay I'm stumped on how to implement this without multiple Param support (#7490). The call site in my draft PR looks like: await Get[Targets](AddressSpecs([DescendantAddresses(..)]) We would either need to add a second param like (Note that this isn't an issue with -- So I think two paths forward to resolving my issue of the Pants repo complaining that
|
It seems like it could be a field of |
Ah. Yep. I thought |
If that would have been true there is nothing preventing adding fields to a |
Correct. At that point, it should probably go back to being a normal dataclass, though. |
…error behavior # Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]
::
does not match any targetsDescendantAddresses
so that call sites can no-op when no matches
Okay, ready to go. I adding the field to |
For dep inference, we need to use
DescendantAddress(src_root)
for every source root, but this will sometimes fail. For example, Pants has a source root3rdparty/protobuf
, but doesn't have any BUILD files in it.We may end up wanting to change the overall CI behavior so that
./pants list ::
would stop failing when no targets exist, but we do not yet want to change the CI behavior until further discussion.So, this adds an optional field to
DescendantAddresses
so that call sites can determine the behavior they want.[ci skip-rust-tests]
[ci skip-jvm-tests]