search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.27k stars 1.58k forks source link

Consider removing `Remove break` (single) assist from the list #56680

Open FMorschel opened 2 months ago

FMorschel commented 2 months ago

Consider removing the Remove break (single) assist from the list.

365731337-29b80121-0b9f-45d0-9ac6-a90dfe58d07e

There is little to no (I could not find any) gain in removing a single break statement. I'm not sure if there are any other lints that we should consider doing the same treatment. Where there would be few to no gains in leaving the single-fix option in the list.

Inspired by https://github.com/dart-lang/sdk/issues/56371.

dart-github-bot commented 2 months ago

Summary: The "Remove break (single)" assist in the Dart editor provides little benefit for removing a single break statement. The user suggests removing this assist from the list of available options as it offers minimal value.

FMorschel commented 1 month ago

https://dart-review.googlesource.com/c/sdk/+/387825

srawlins commented 1 month ago

@bwilkerson any precedent for this? Only offering a fix as an "across file" fix, and not in an individual position?

bwilkerson commented 1 month ago

I can't think of any other fixes that can only be applied across the whole file and not in a single location.

If we really believe that there's no value in applying it individually then I could see an argument for not including the option in the menu (to reduce clutter), but there's also an argument to be made for consistency. Not sure which is the stronger design principle in this case.

However, the current implementation will only present a whole-file fix if there's more than one diagnostic in the file. Unless we change that behavior I believe that the suggested change will have the effect of removing the fix in the case where there's only one such diagnostic being reported in a file. I don't think that would be a good idea. (I haven't looked at the CL, so it's possible that concern has already been dealt with.)

FMorschel commented 1 month ago

Reading your answer and the current implementation (and check failure) of the CL, I don't think the approach I took was the correct one.

What if we added a new CorrectionApplicability option or new propriety in ResolvedCorrectionProducer to indicate that "if there is more than one, show only the multi fix"? I think this would be a better solution to this. WDYT?

bwilkerson commented 1 month ago

I agree that adding an applicability of "if there is more than one, show only the multi fix" would be a better approach. The only question in my mind is whether the value is greater than the cost (of both implementation and of maintenance).