nbadal / ktlint-intellij-plugin

Ktlint plugin for IntelliJ IDEA + Android Studio
MIT License
158 stars 23 forks source link

Lint violations which can be autocorrected should be visible on request #399

Closed Vampire closed 9 months ago

Vampire commented 9 months ago

While in 0.20.0-beta-4 you had findings shown as weak warning, warning, or error according to enabled-state of the plugin, in 0.20.0-beta-5 neither of the three is shown anymore.

paul-dingemans commented 9 months ago

Yes, that is on purpose. Please see #394. There should a warning containing a summary of the number of violations which were found and can be autocorrected. Screenshot 2023-12-04 at 20 52 38

unconfigured (only showing the total number of violations that ktlint can detect)

disabled (only showing the total number of violations that ktlint can detect) Screenshot 2023-12-04 at 20 49 35

enabled (showing each error that can not be autocorrected and a total number of problems that can be autocorrected) Screenshot 2023-12-04 at 20 47 44

Vampire commented 9 months ago

Oh, that's very sad. :-( With ktlint "unconfigured" or "disabled", I liked that I get a second opinion what could be improved that I could follow or not at my will. How about instead having entries in Inspections, one for each level, that are set to "No highlighting", so that a user can set them to "Error", "Warning", and "Weak Warning" if he want them displayed?

paul-dingemans commented 9 months ago

Oh, that's very sad. :-( With ktlint "unconfigured" or "disabled", I liked that I get a second opinion what could be improved that I could follow or not at my will.

Based on issue #380 you created earlier, I understood that you did not want to see violations whenever the plugin was not (yet) enabled.

Another reason for the new behavior is that it also showed details about violations that can be autocorrected. One of the main reasons for changing the UX of the plugin is that the old plugin could lead to wast of developer time whenever a developer starts resolving violations which can be autocorrected. I want the plugin to behave as stealthy as possible, and with minimal configuration.

How about instead having entries in Inspections, one for each level, that are set to "No highlighting", so that a user can set them to "Error", "Warning", and "Weak Warning" if he want them displayed?

I am not sure whether I understand what you mean. Basically there are two types of violations. Violations that need to be resolved manually, and violations that can be autocorrected.

Vampire commented 9 months ago

Based on issue https://github.com/nbadal/ktlint-intellij-plugin/issues/380 you created earlier, I understood that you did not want to see violations whenever the plugin was not (yet) enabled.

Interesting, what did I say to make you think that? I there just meant to complain that a "Reformat Code" built-in action triggered a ktlint format, even though the plugin was not in enabled state.

I am not sure whether I understand what you mean. Basically there are two types of violations. Violations that need to be resolved manually, and violations that can be autocorrected.

In the case you don't like people to use, having the plugin not configured or not enabled. :-) Things could of course be autocorrected if the ktlint format is triggered manually. But for control freaks like me that don't like ktlint on built-in-format and for sure not as on-save action, it was actually nice to see the ktlint warnings as suggestions to change the code format as a kind of second opinion.

Now you cannot see them anymore. What I suggested was not less stealthy by default and not needing more configuration by default, but providing the possibility to configure how you like it.

If you open the "Inspections" settings in IntelliJ, you see that there are many inspections and you can customize for each the severity and enabled state to your liking. So you could have one inspection "weak ktlint warning", one "ktlint warning", and one "ktlint error" that are then used for the three cases (disabled, unconfigured, enabled) and that you by default set to severity "No highlighting" to be stealthy. And strange people like me can then raise the severity of "weak ktlint warning" to "weak warning", "ktlint warning" to "warning", and "ktlint error" to "error".

paul-dingemans commented 9 months ago

Based on issue #380 you created earlier, I understood that you did not want to see violations whenever the plugin was not (yet) enabled.

Interesting, what did I say to make you think that?

Well, I have to admit that there some free interpretation there. Indeed, you mentioned that format was executed while the plugin was not enabled. We both do agree, that that should not happen.

In #356 you mentioned:

Even if I invoke the IntelliJ formatter I don't necessarily want ktlint to interfere, as I might like IntelliJ result better. Even if you consider that bikeshedding, it isn't. A project can for example have as anti-bikeshedding rule to use the IJ formatter, but anyone just having ktlint installed will then suddenly change the code.

So, I wanted to make it possible that you can decide per project whether you want ktlint to be run on the project or not. My consideration was that if you do not want to run ktlint for a given project, that you also should not be overwhelmed by all ktlint violations that will be found in the project.

But for control freaks like me that don't like ktlint on built-in-format and for sure not as on-save action, it was actually nice to see the ktlint warnings as suggestions to change the code format as a kind of second opinion.

It is a kind of funny that we are at opposite sites of the spectrum of trusting ktlint. I only look at formatted output of ktlint when I am adding or changing ktlint rules. I am glad that you are advocating this different angle at using the plugin. Eventually, the plugin will become better when we are both happy with the plugin.

If you open the "Inspections" settings in IntelliJ, you see that there are many inspections and you can customize for each the severity and enabled state to your liking. So you could have one inspection "weak ktlint warning", one "ktlint warning", and one "ktlint error" that are then used for the three cases (disabled, unconfigured, enabled) and that you by default set to severity "No highlighting" to be stealthy. And strange people like me can then raise the severity of "weak ktlint warning" to "weak warning", "ktlint warning" to "warning", and "ktlint error" to "error".

I don't think that I am willing to go down that road. In my opinion this level of configuration is too finegrained. It is harder to test, maintain and to explain to others.

I am willing to consider to add an option to disable automatic formatting of code when invoking the default formatting of intellij. Formatting on save can already be disabled.

Vampire commented 9 months ago

So you mean that then I would enable ktlint but disable formatting on save and formatting on format? What would that change? You disabled all findings even in enabled state, so the behavior would then be identical to now with the plugin unconfigured or disabled, or not? No findings shown except for the catch-all finding on the first character and being able to trigger ktlint manually.

paul-dingemans commented 9 months ago

So you mean that then I would enable ktlint but disable formatting on save and formatting on format? What would that change?

Yeah, I should have mentioned that this then will show all ktlint violations like it did before. Also, it will still be possible to invoke the ktlint format manually.

Vampire commented 9 months ago

Ok, then it sounds pretty good, yeah. :-) The manual ktlint format should always work now anyway even if disabled, this hopefully stays like that.

paul-dingemans commented 9 months ago

I have reopened the issue as it provided valuable insights, in what use case this is relevant.