https://pantsbuild.org/ logo
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