https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-father-404

11/10/2020, 8:02 PM
I found an awkward UX bug:
Copy code
▶ ./pants test src/python/pants/util/strutil_test.py src/python/pants/util:tests
...
✓ src/python/pants/util/strutil_test.py:tests succeeded.
✓ src/python/pants/util/strutil_test.py:tests succeeded.
That is, the result shows up twice for the same file address. That’s because
TargetRootsToFieldSets
stores a mapping of
TargetsWithOrigin
to
FieldSet
, and this is the same target but two distinct origins The fix is simple for this if we can get rid of
TargetsWithOrigin
et al. We no longer use that mechanism anymore, and it has some weight to support. What do we think about shedding the ability to use
WithOrigin
? In the past, we used it for globbing semantics, to only error if a certain type of spec was used. Since then, we’ve made globbing semantics more uniform to never error and only ever run on things we are able to
cc @happy-kitchen-89482 if you’re fine with removing
WithOrigin
w

witty-crayon-22786

11/10/2020, 8:04 PM
is this “removing” it, or this “using WithOrigin in fewer places: only those that need origins”?
h

hundreds-father-404

11/10/2020, 8:05 PM
To fix this bug, we only need to lose the
targets_with_origins
property from
TargetRootsToFieldSets
. But imo, if we want to keep
WithOrigins
in general, we should probably have it there too Right now we have: *
AddressesWithOrigins
*
TargetsWithOrigins
*
TargetRootsToFieldSets.targets_with_rigins
My two cents: 1. It’s good to delete code like this. We have a lot of abstractions already, and this will simplify things a bit. Also, less wasted work for users; these rules always run when resolving specs 2. It used to be a helpful mechanism, but our UX design has evolved to where we are now. It’s no longer relevant. 3. If a plugin author needs it in the future, they can add back the logic
4. we didn’t deem this mechanism worthy enough to document at https://www.pantsbuild.org/v2.1/docs/rules-api-and-target-api
w

witty-crayon-22786

11/10/2020, 9:31 PM
yea, if the existing usecases don’t meet the bar (or there aren’t any) then deleting code is great.
h

hundreds-father-404

11/10/2020, 9:32 PM
The only use case I’ve found is to echo back to the user what specs they ran with, but we can solve that another way
Deleting WithOrigins is a big change, and we want to cherry-pick..I’ll split out fixing this UX bug vs. the overall change
w

witty-crayon-22786

11/10/2020, 9:34 PM
hm. do we really need to cherry-pick?
h

hundreds-father-404

11/10/2020, 9:34 PM
True
w

witty-crayon-22786

11/10/2020, 9:34 PM
it’s not ideal, but it also seems ok to fix forward.
h

hundreds-father-404

11/10/2020, 9:34 PM
Yeah it doesn’t break anything, only is confusing UX