Standardize how we reference formatters, linters, and checkers in goal summary

[Eric-Arellano]

Problem

Soon we're adding lint --only=<tool>, so the name of these tools will become part of our API. Some of the tools currently have spaces, which means you need quotes with check --only='go compile'. Generally, we're (intentionally!) inconsistent with how we capitalize.

Because of inconsistent capitalization, our final summaries also don't sort how users might expect.

Before

Note that autoflake shows up as the 4th entry, not the 1st.

❯ ./pants lint src/python/pants/util/strutil.py
17:14:06.98 [INFO] Completed: Lint with regex patterns - regex-lint succeeded.
17:14:08.43 [INFO] Completed: Lint with Flake8 - Flake8 succeeded.                                                                              
17:14:08.43 [INFO] Completed: Lint with Black - Black succeeded.                                                                                
17:14:08.50 [INFO] Completed: Lint with docformatter - Docformatter succeeded.                                                                  
17:14:08.50 [INFO] Completed: Lint with autoflake - autoflake succeeded.                                                                        
17:14:08.50 [INFO] Completed: Lint with isort - isort succeeded.

✓ Black succeeded.
✓ Docformatter succeeded.
✓ Flake8 succeeded.
✓ autoflake succeeded.
✓ isort succeeded.
✓ regex-lint succeeded.

After

❯ ./pants lint src/python/pants/util/strutil.py
17:10:41.10 [INFO] Completed: Lint with regex patterns - regex-lint succeeded.
17:10:42.46 [INFO] Completed: Lint with Flake8 - flake8 succeeded.                                                                              
17:10:42.46 [INFO] Completed: Lint with Black - black succeeded.                                                                                
17:10:42.54 [INFO] Completed: Lint with docformatter - docformatter succeeded.                                                                  
17:10:42.54 [INFO] Completed: Lint with autoflake - autoflake succeeded.                                                                        
17:10:42.54 [INFO] Completed: Lint with isort - isort succeeded.                                                                                

✓ autoflake succeeded.
✓ black succeeded.
✓ docformatter succeeded.
✓ flake8 succeeded.
✓ isort succeeded.
✓ regex-lint succeeded.

[ci skip-rust]

[kaos]

I see three obvious options here (somewhat in my order of preference):

a) Explicitly name each use of a tool, i.e. have separate fields for presentation and api.

class MyTool (Subsystem):
    api_name = "my-tool"
    display_name = "My Tool"

b) Derive the display name from the api name.

class MyTool (Subsystem):
    api_name = "my-tool"
    # Infer display_name i.e. replace `-` => ` ` and then capitalize each word.

c) Derive the api name from the display name.

class MyTool (Subsystem):
    display_name = "My Tool"
    # Infer api_name i.e. replace ` ` => `-` and then lowercase the whole thing.

I would prefer d b (thanks @thejcannon) over c (to ensure a stable predictable api), while a is a bit more verbose, but gives the most control. A mix of a & b could be achieved, if the name inference is overridable so you can customize it.

[thejcannon]

(Non-blocking, this PR is an improvement so it belongs in)
This is the odd duckling, which got me wondering what these are used for.

AFAICT (and mypy backs me up) they are only used on the superclass for debug_hint. That could probably get away with using cls.__name__ 🤷‍♂️ . Then the only remaining usages are in each tool's @rules as a DRY way of using the name (E.g. formatter_name=request.name). In that case, we could reference the subsystem.options_scope in all cases except this one, where we could hardcode it.

Of course, instead of an implicit convention (which this now has), in the future #14238 could implement a helper method which then allows for a codified convention 😉

[Eric-Arellano]

See #14356, which this is all prework for.

[thejcannon]

I see. It'd be nice if these things were more automagical if the client followed a convention (much like how Django operates for a lot of things).

But I'm just soapboxing now 😄

[Eric-Arellano]

have separate fields for presentation and API.

I'm not following why we would differentiate between display name vs API name? The help message for --lint-only instructs people to look at the final summary for the name to use: #14356. I think it would be confusing to have both.

Infer display_name i.e. replace - => and then capitalize each word.

That's tricky because we don't want to capitalize each word. isort should always be isort, not Isort. MyPy should be MyPy, not Mypy.

Infer api_name i.e. replace => - and then lowercase the whole thing.

That could work, although I don't think we need to differentiate display vs API.

[kaos]

I don't think we need to differentiate display vs API.

Yeah, in that case I don't see any issues. Guess I went down the wrong hole by misinterpreting the meaning of "Generally, we're inconsistent with how we capitalize." ;)

[Eric-Arellano]

Guess I went down the wrong hole by misinterpreting the meaning of "Generally, we're inconsistent with how we capitalize." ;)

Oh yeah, I should have clarified "intentionally inconsistent" :)

[stuhood]

I love consistency, but one other option that wasn't mentioned in @kaos's list:

I see three obvious options here (somewhat in my order of preference):
[...]

We could also make the match fuzzy/partial/case-insensitive, similar to pytest's -k flag (and other test-runner flags).

That doesn't mean that the names aren't a public API... just that it might not matter how long/short/stylized they are.

[Eric-Arellano]

We could also make the match fuzzy/partial/case-insensitive, similar to pytest's -k flag (and other test-runner flags).

Agreed. At a minimum, don't care about capitalization. Maybe allow spaces to be substituted with - or _ or no space..I took that approach with #14356 at first, but it felt more complex than necessary - like documenting all those affordances in the help.

So I went for the simpler approach of consistency. Which has the added benefit of the final summary being sorted better. (Although I guess we could sort by name.lower())

[Eric-Arellano]

What do folks think? Am I okay to merge this PR as is?

Fwit, if we in the future decide we do care about the display name going back to some capitalization, we could change them back and add the fuzzing/insensitivity? (I personally don't find the capitalization valuable enough to do that though, yay consistency)

[thejcannon]

I'm actually finding we're kind of duplicating these strings in several places. I have some thoughts on reducing duplication.
At a minimum we use:

• The display name in strings
• The options scope for options
• the request name, which is pretty much only needed to reduce duplication (and in most places, the code has access to the subsystem)

So I think @kaos 's option C would likely reduce this list down to 1 declaration in most cases.

[Eric-Arellano]

I'm actually finding we're kind of duplicating these strings in several places

We shouldn't be anymore? We now only define the string once: Tool.options_scope. Then we propagate that to:

• tool lockfile's "resolve" name
• FormatRequest, used for --only + debug_hint
• FormatResult, used for the final summary

I don't think the propagation is a big deal? What I didn't like was setting up the string as a new value multiple times, which we used to do.