Rename applicability levels to always, sometimes, and never #7821
Conversation
zanieb
changed the title
zanieb
changed the title
zanieb
force-pushed
the
zanie/app-refactor-freq
branch
from
180c6e3
to
99d6301
Compare
|
I'm still not certain these are the best way to express the ideas here, but they are pretty easy to rename in the future and I'd like to move forward to unblock #7769 |
| Applicability::Suggested => "Suggested fix", | ||
| Applicability::Manual => "Possible fix", | ||
| Applicability::Unspecified => "Suggested fix", /* For backwards compatibility, unspecified fixes are 'suggested' */ | ||
| // TODO(zanieb): Adjust this messaging once it's user-facing |
There was a problem hiding this comment.
What do you think is the outcome here? Like what language?
There was a problem hiding this comment.
I'm not sure! Perhaps what I have now with some additional commentary such as the use of --unsafe-fixes to enable the fix.
| @@ -365,7 +367,7 @@ pub(crate) fn trailing_commas( | |||
| // removing any brackets in the same linter pass - doing both at the same time could | |||
| // lead to a syntax error. | |||
| let contents = locator.slice(missing_comma.1); | |||
| diagnostic.set_fix(Fix::automatic(Edit::range_replacement( | |||
| diagnostic.set_fix(Fix::always_applies(Edit::range_replacement( | |||
There was a problem hiding this comment.
I still prefer Fix::always and Fix::suggested over Fix::always_applies and Fix::sometimes_applies. Isn't the _applies implied?
There was a problem hiding this comment.
We'd then have Fix::always_edits for the multi-edit case which is admittedly strange, but is that why we're including _applies here?
There was a problem hiding this comment.
I'm leaning towards explicit since there was some confusion expressed about the previous methods. I feel like with a bare "always" it's unclear what it refers to and could be confused with the availability?
CodSpeed Performance ReportMerging #7821 will degrade performances by 2.49%Comparing Summary
Benchmarks breakdown
|
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
After working with the previous change in #7821 I found the names a bit unclear and their relationship with the user-facing API muddied. Since the applicability is exposed to the user directly in our JSON output, I think it's important that these names align with our configuration options. I've replaced `Manual` or `Never` with `Display` which captures our intent for these fixes (only for display). Here, we create room for future levels, such as `HasPlaceholders`, which wouldn't fit into the `Always`/`Sometimes`/`Never` levels. Unlike #7819, this retains the flat enum structure which is easier to work with.
Following much discussion for #4181 at #5119, #5476, #7769, #7819, and in [Discord](https://discord.com/channels/1039017663004942429/1082324250112823306/1159144114231709746), this pull request changes `Applicability` from using `Automatic`, `Suggested`, and `Manual` to `Always`, `Sometimes`, and `Never`. Also removes `Applicability::Unspecified` (replacing #7792).
After working with the previous change in #7821 I found the names a bit unclear and their relationship with the user-facing API muddied. Since the applicability is exposed to the user directly in our JSON output, I think it's important that these names align with our configuration options. I've replaced `Manual` or `Never` with `Display` which captures our intent for these fixes (only for display). Here, we create room for future levels, such as `HasPlaceholders`, which wouldn't fit into the `Always`/`Sometimes`/`Never` levels. Unlike #7819, this retains the flat enum structure which is easier to work with.
Following much discussion for #4181 at #5119, #5476, #7769, #7819, and in Discord, this pull request changes
Applicabilityfrom usingAutomatic,Suggested, andManualtoAlways,Sometimes, andNever.Also removes
Applicability::Unspecified(replacing #7792).