https://pantsbuild.org/ logo
Powered by
h

hundreds-father-404

10/13/2021, 12:25 AM
Case study of how CLI specs are broken currently, and what I think could make sense: https://docs.google.com/document/d/1WWQM-X6kHoSCKwItqf61NiKFWNSlpnTC5QNu3ul9RDk/edit#
đź‘€ 1
@happy-kitchen-89482 @witty-crayon-22786 a question on semantics that is confusing this implementation a lot: should 
./pants count-loc ::
count all files, or only files owned by targets? https://docs.google.com/document/d/1WWQM-X6kHoSCKwItqf61NiKFWNSlpnTC5QNu3ul9RDk/edit#heading=h.kwc67nd5ho7m
w

witty-crayon-22786

10/13/2021, 9:09 PM
i think that matching things that aren’t owned by targets is the reason filesystem globs exist
so 
::
not matching unowned files seems fine
h

hundreds-father-404

10/13/2021, 9:10 PM
It is, which is the heart of this question. We have said that 
dir
is a synonym for 
dir:
, and 
dir::
is the recursive version of that So, are we sticking to 
dir:
and 
dir::
being "address" specs, or they're really now dir specs? (If the latter, I've been pondering for last 15 minutes if address specs vs. filesystem specs even makes sense and we somehow merge Specs)
Life is much, much easier if we say 
./pants count-loc ::
is only sources owned by targets
NB that we already have 
"**/*"
to mean "all sources, regardless of targets". So I don't think we need to change 
::
to now do that
But this does mean that 
./pants count-loc path/to/dir
will fail if there are no targets in that dir. You'd have to use 
./pants count-loc 'path/to/dir/*'
w

witty-crayon-22786

10/13/2021, 9:15 PM
how did this case work before…? i’m not sure what 
count-loc $dir
would have done before
h

hundreds-father-404

10/13/2021, 9:15 PM
Before, $dir is shorthand for 
dir:dir
, so it errors
w

witty-crayon-22786

10/13/2021, 9:18 PM
ok… so that seems like an improvement then, independent of what we do for filesystem globs
basically, it seems like directories were always a bit ambiguous when deciding between AddressSpecs and FilesystemSpecs
h

hundreds-father-404

10/13/2021, 9:21 PM
To be clear, 
./pants count-loc dir
would still error if there were no targets, as 
dir
is short for 
dir:
now It could be that we decide we normally want 
dir
to act like 
dir:
, but we make an exception for 
validate
and 
sources-snapshot
? You use 
dir:
if you only want owned sources? That might work. And then I guess you must use 
'dir/**/*'
for recursive case, given that we agreed it's not worth it to add 
dir/...
w

witty-crayon-22786

10/13/2021, 9:22 PM
i think that that amounts to making 
Specs
expansion dependent on whether you’re requesting files or targets… which is maybe fine. but right now that’s decided statically at parse time, right?
âž• 1
h

hundreds-father-404

10/13/2021, 9:23 PM
Where this gets tricky is our onboarding docs first introduce arguments with 
count-loc
! It's great because you don't need any BUILD files to try it Benjy understandably wants to rewrite it to use files, dirs, and I would imagine 
dir::
, which are the only 3 address forms you typically would need. (Plus sometimes address literal)
w

witty-crayon-22786

10/13/2021, 9:24 PM
@hundreds-father-404: but really, this seems like something that can be decoupled from changes to 
Specs
… it’s more to do with those goals than anything else.
basically, changing 
dir
to mean “all targets in dir” seems strictly like a step forward for this case, and the fact that it’s always been ambiguous whether 
dir
was a filesystem spec or an address spec isn’t changing here
h

hundreds-father-404

10/13/2021, 9:25 PM
which is maybe fine. but right now that’s decided statically at parse time, right?
Yeah. I think we would have a new rule somehow wrapping 
Specs -> Addresses
, which allows you to set whether to convert 
DirLiteralSpec
into 
SiblingAddressSpec
or keep it that. Then a rule that wraps that to return a singleton, such that most callers don't need any changes at all
Okay, I think that's true that we can implement most of the proposal via AddressSpecs without having to account for these two goals because I think it will indeed be possible in the future to let rules change how certain Specs are interpreted
w

witty-crayon-22786

10/13/2021, 9:27 PM
well… or lazily decide how to interpret it
âž• 1
h

happy-kitchen-89482

10/13/2021, 9:28 PM
I think it would be really confusing for 
./pants count-loc ::
not to consider files that aren't owned by a target
đź‘Ť 1
Generally, targets are dumb and we should not require them except where we have to
BUILD file targets that is
w

witty-crayon-22786

10/13/2021, 9:29 PM
maybe. but that seems a bit orthogonal to this change… if we can avoid changing how filesystem specs work until we can actually replace them, that would be ideal (imo).
if 
::
was going to match all files, then we should remove support for `**`… and that seems like more than we should bite off here.
h

hundreds-father-404

10/13/2021, 9:31 PM
That's I think what I was getting at, Stu. Whether it still makes sense to split 
FilesystemSpecs
and 
AddressSpecs
in today's world. Definitely it does make sense to have the classes 
FileLiteralSpec
and 
AddressLiteralSpec
etc, i.e. preserve precisely what the user specified. Then we can lazily interpret the semantics from there. But, yeah, whether we have separate rules for those collection types, vs. everything being in 
Specs
I agree that I think we can punt on that implementation. I thought I needed it for prework
h

happy-kitchen-89482

10/13/2021, 9:38 PM
if 
::
 was going to match all files, then we should remove support for `**`… and that seems like more than we should bite off here.
I don't get the logic
We can leave 
**
until we get around to removing support for it
It's not doing harm
đź‘Ť 1
I agree that it is superfluous if 
::
works the way I want it to
But removing it isn't a priority
h

hundreds-father-404

10/13/2021, 9:44 PM
It's not doing harm
Yeah, not a ton of code to support it. And we don't need to document it either
Powered by