https://pantsbuild.org/ logo
Join Slack
Powered by
I'm having trouble finding the right seam to inser...
# development
b
I'm having trouble finding the right seam to insert a deprecation of secondary ownership. Because realistically the only place we use in the core backends is 
package
for Python for AWS Lambda and Google Cloud, but it could be used anywhere and where it is resolved is very far removed from where it is used šŸ˜
From `src/python/pants/backend/google_cloud_function/python/target_types.py`'s 
filespec
Copy code
if not path.endswith(".py"):
            return {"includes": []}
haha you can't have Python files as Google/AWS cloud handlers unless they end in 
.py
womp womp
šŸ˜® 1
I want to make sure that commands like `package ::`don't warn
w
hm.
i think that the relevant usage of 
OwnersRequest
is in 
src/python/pants/engine/internals/specs_rules.py
you could maybe do something like looking at the count of files going in to decide whether itā€™s a request worth warning for?
but that could still have some false positives i think.
on the other hand: itā€™s a warning. false positives for the warning is probably ok, since you wonā€™t get false positives once the deprecation is removed.
b
Yeah no good way. I really wish we could just remove šŸ˜‘
Turns out 
paths
arguments can't be used if it matches secondary fields šŸ«£
I don't think counts works either. I'm wondering if anyone does --changed-since with 
package
.
w
almost certainly.
b
We might not be able to remove then. Not without a good workaround
w
well, thatā€™s what iā€™m saying about the warning vs error
some false positives in the warning might be ok, since they donā€™t block anyone.
b
It's annoying it's only used for package , and for Python, and for only 2 target types. Maybe my paths issue is just an issue with paths, but I get the sense secondary ownership is a bit of an afterthought
Maybe we remove it for run, and make secondary ownership not the default, then goals opt in?
w
hm. iā€™m not understanding the 
run
vs 
package
distinction that you are making.
b
Well for run, we have to pick one target so we try and disambiguate. Although nothing in our repo has the ambiguity anymore, so just for plugins
For 
paths
the args error if you give it a filename that has a secondary owner and the error message literally suggests using the arg provided šŸ¤¦ā€ā™‚ļø
Then for all other goals that can work on multiple targets, like `package`theres no problem
w
For 
paths
the args error if you give it a filename that has a secondary owner and the error message literally suggests using the arg provided šŸ¤¦ā€ā™‚ļø
weā€™re no longer talking about the deprecation then? instead, the actual removal of 
SecondaryOwnerMixin
?
b
Yeah that would be ideal. Since it's only used by 2 targets and leads to ambiguities
w
sure, iā€™m fine with removing it. just trying to understand what weā€™re talking aboutā€¦ the deprecation, or the removal.
the deprecation cannot cause errorsā€¦ it might make some errors more confusing if the warning triggers as a false positive
b
We can't remove without first warning, so that's what I want to tackle first
w
ok. then i donā€™t understand the 
paths
case.
is the warning that is triggered there not valid?
b
No it isn't valid. And I'm just using that as an example of where the current behavior is hazardous
Well maybe valid, but extremely misleading
w
and by ā€œcurrent behaviorā€, you mean the ā€œcurrent behavior of the deprecation branch that you haveā€?
b
No. Ah yeah that's where we've diverged. There's an error today on mainline pants. If there's a secondary owner of a file, 
paths
will error about ambiguity.
And one of the suggestions is the file used in the command (ergo misleading/unhelpful)
w
sure, fine: i agree that we should deprecate it.
heh
b
And I bring it up because it shows secondary ownership is hazardous. So either we pull or we go all in
w
yes, pull, agreed
b
Hehe so now how to deprecate reasonably well is the question šŸ˜‚
w
i think that i gave one answer in my first few posts in the thread
b
I don't like false positives because the error message is like "something might be removed, do this instead" but there isn't an issue. We scared the user for nothing
I guess we could determine if the returned ownership has only secondary for a filespec. And if so error
Let me try that
Oh wait, that's not possible, because we'll always match primary šŸ¤¦ā€ā™‚ļø
Yeah I don't think this is really doable without also modifying the caller's site. E>g. 
package
Open in Slack
PreviousNext
Powered by