Add description for the validate goal. #9602
Conversation
So that `./pants goals` displays something useful for it. Also improves descriptions of a couple of other goals. [ci skip-rust-tests] [ci skip-jvm-tests]
| @@ -25,7 +25,7 @@ class DependencyType(Enum): | |||
|
|
|||
| # TODO(#8762) Get this rule to feature parity with the dependencies task. | |||
| class DependenciesOptions(LineOriented, GoalSubsystem): | |||
| """Print the target's dependencies.""" | |||
| """List the transitive dependencies of the input targets.""" | |||
There was a problem hiding this comment.
Not necessarily transitive. We default to --no-transitive.
There was a problem hiding this comment.
Hmmm, the help text for --transitive is wrong, it looks like:
"Run dependencies against transitive dependencies of targets specified on the command line."
That sounds like "run this goal on every target in the closure". But looking at how the option is used in the code, I think this should say
"Show all transitive dependencies. If unspecified, show only direct dependencies."
I'll make that change (and to the goal description) but please confirm my understanding.
There was a problem hiding this comment.
Ah, yes, your read is correct. --filedeps-transitive could be improved too to reflect this. core/project_info/filedeps.py.
[ci skip-rust-tests] [ci skip-jvm-tests]
[ci skip-rust-tests] [ci skip-jvm-tests]
[ci skip-rust-tests] [ci skip-jvm-tests]
| @@ -34,6 +34,8 @@ class DetailLevel(Enum): | |||
|
|
|||
|
|
|||
| class ValidateOptions(GoalSubsystem): | |||
| """Validate sources against regexes.""" | |||
There was a problem hiding this comment.
So, I think that we put this here initially because the lint goal hadn't been ported to v2. It would be good to port this rather than making "validate" first class, if possible.
There was a problem hiding this comment.
The reason we do not want to do this is that validate works with any arbitrary file, even if there is no owning target, thanks to SourcesSnapshot. Meanwhile, lint only works with files with owning targets.
Toolchain has come to depend on this ability to do things like validate BUILD files, which usually have no owner.
lint is already extremely complex - it would probably be possible, but would be horrible to rewrite it to work with either SourcesSnapshots or Targets.
--
Instead, I recommend that we rename the goal validate to something like regex-validate. It's confusing what validate vs. lint mean. validate sounds more generic than it really is.
We would get rid of the sourcefile-validation subsystem and move the options into the GoalSubystem.
There was a problem hiding this comment.
In general, I think that we need to bias toward having fewer goals, each of which is fundamentally useful for some purpose.
I'd almost rather include validate as a special case of lint then have a separate goal for "per-file lint".
There was a problem hiding this comment.
I'd almost rather include validate as a special case of lint then have a separate goal for "per-file lint".
Meaning, change lint.py to accommodate both traditional target-based linters and linters that solely need files, like this?
Generally, I agree that I would rather make our code more complex if it means users have a much nicer experience. It would be nice to get regex-validate working with lint.
--
This gets me thinking again. Black, isort, and docformatter don't need to care at all about targets, but they do. (Meanwhile, Flake8, Pylint, and Bandit do care about targets because they need interpreter compatibility). Maybe a stretch goal is to be able to run Black on any arbitrary Python file in the build root, even if there are no targets.
There was a problem hiding this comment.
All great thoughts, but out of scope for this change, which is literally just about adding a description to an existing goal...
There was a problem hiding this comment.
I think having this functionality in lint makes sense, although it is a good point that it can operate on literally any file, including BUILD files, so I'm not sure how hard that would be.
[ci skip-rust-tests] [ci skip-jvm-tests]
So that
./pants goalsdisplays something useful for it.Also improves descriptions of a couple of other goals.
[ci skip-rust-tests]
[ci skip-jvm-tests]