Improve formatting of ./pants goals and ./pants target-types2
#9414
Conversation
# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # No Rust changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.
# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # No Rust changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.
# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # No Rust changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.
Eric-Arellano
requested review from
jsirois,
benjyw,
pierrechevalier83 and
blorente
There was a problem hiding this comment.
A downside of this PR is that the output of both goals is now 3x longer and ./pants goals no longer fits on my terminal without scrolling.
But, I think the gains in readability are worth it. The biggest thing I wanted to get rid of was using rjust() because it makes it difficult to quickly scan all the goal and target type names.
|
We could still have the shorter goals output but left-justify the goal name. I think that might be better. |
[ci skip-jvm-tests] # No JVM changes made. [ci skip-rust-tests]
# Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.
Good call. That looks better. -- I think I want to add line wrapping of the descriptions before merging this. The soft wrapping makes the first "after" picture not look great. |
Without this, several `goal` descriptions use soft wrapping, which puts a bunch of noise in the left column which is supposed to solely be the name of goals and target types. # Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # No Rust changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.
|
Here's a 3rd option, which I think is my favorite. Tries to strike a balance between readability vs. compactness.
Thoughts? |
|
I find Option 3 to be the most visually pleasing, and worth the cost of extra space. My eyes can easily navigate it whereas the previous option's right-indent when wrapping on a new line felt unexpected - it didn't read like typical everyday copy. |
I personally like that 3rd option more than the alternatives, but I wouldn't be massively anal if anyone disagreed strongly enough. |
…on indentation Also, stop using () with target types, e.g. `files()` -> `files`. Pierre pointed out that this was confusing because you can't type `./pants target-types2 --details=files()`. # Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # No Rust changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.
| chars_before_description = longest_target_alias + 2 | ||
| alias = console.cyan(f"{self.alias}".ljust(chars_before_description)) | ||
| if not self.description: | ||
| description = "<no description>" |
There was a problem hiding this comment.
Not sure I love this. Maybe empty would look more user friendly. Here in a way it feels like punishing the user of the tool for the developer not taking the time to provide a description.
There was a problem hiding this comment.
Sgtm, I agree.
There was a problem hiding this comment.
Oh, hm, @gshuflin explicitly added this feature in #8088 and I remember it confused him why something was blank, e.g. wondering if it was a bug in Pants.
I would rather that we place a higher burden on us Pants devs to ensure we're writing good generated documentation. For example, when adding a new goal, we should expect Pants devs to put a screenshot of how it renders in ./pants goals.
One benefit of <no description> is that it's uglier than a blank line, so it motivates us Pants devs to do something. Specifically because of seeing <no description>, I've added docstring to two goals in the past two months. I probably would have done nothing if it were just an empty line.
| name = name.ljust(chars_before_description) | ||
| if self._use_color: | ||
| name = cyan(name) | ||
| description_lines = textwrap.wrap(description, 80 - chars_before_description) |
There was a problem hiding this comment.
I would prefer for this 80 to be demagicked
There was a problem hiding this comment.
One followup PR I'm planning is to add a util so that it's easy for us to consistently wrap exception messages, whereas we currently almost always use soft wrapping. I've changed my mind that I now think hard wrapping is a better UX in a CLI app than soft wrapping.
I think we'd want the 80 demagicking to happen in that PR. Right now, we're duplicating 80 in 2 places, and once I add ./pants backends, 3 places. Imo, it's premature to factor out the 80 constant without also factoring out the overall wrapping mechanism in a followup.
Before:
After: