Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add maxWidth option to IDEA plugin, fixes #351 #492

Closed

Conversation

robinpokorny
Copy link

@robinpokorny robinpokorny commented Jul 4, 2024

Screenshot 2024-07-04 at 15 34 05

This add a new override option for maxWidth, one of the most changed configs, see #351.

Unfortunately, working with Kotlin data classes is less than ideal from Java, so new FormattingOptions have to be created.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 4, 2024
@simonhauck
Copy link

If this is merged, it should in my opinion also be available via the CLI.

If not, people that are not using the IntelliJ plugin can not achive the same formatting as IntelliJ users. Also it could be relevant for CI applications.

@robinpokorny
Copy link
Author

If this is merged, it should in my opinion also be available via the CLI.

If not, people that are not using the IntelliJ plugin can not achive the same formatting as IntelliJ users. Also it could be relevant for CI applications.

The Gradle plugin supports changing the maxWidth. While I personally think it's unfortunate, that this option exists, my change is actually brining more consistency.

@simonhauck
Copy link

simonhauck commented Jul 8, 2024

I am familiar with the gradle plugin and also spotless is offering this configuration. Even in the web playground is this option available.

That being said, those are in my opinion "third party" tools (expect the playground, but that's just a "playground") and not defining the "ktfmt"-standard, which is tracked in this repository. In the README is written:

Note: There is no configurability as to the formatter's algorithm for formatting (apart from the different styles). This is a deliberate design decision to unify our code formatting on a single format.

That indicates for me, that even though both plugins offer this functionality, it is not part of the public API - if that is the case (and I could be wrong), then the official IntelliJ plugin should not offer this option.

If the above mentioned design decision is changed, it should be documented in the README is and then also be available in all artifacts from this repository - the CLI tools and the IntelliJ plugin.

Technical side

From the technical point of view: I know that at least the gradle plugin is mapping their options to the FormattingOptions.kt, which is a non internal class. From a quick look, it seems that the internal modifier is not used consistently in this project, so I would not count on this being part of the public API. If it is, then why shouldn't all options in this class be part of the public API. As you mentioned on slack, this would logically be the next step.

Summary

For me this PR is not just about adding a setting to the plugin. It is a decision if configuration options are introduced in the official API - which should be a conscious and documented decision.

If it is decided, that ktfmt has more options then:

  1. The setting should also implemented for the CLI, the documentation changed, this PR accepted
  2. In the long term the other options should also be available to the CLI and IntelliJ plugin

If it is a clear decision, that this should not be part of ktfmt

  1. Consider making the classes internal, so that plugins can not overwrite it. This is for the plugins a breaking change, but be more aligned with the goal of ktfmt.

In the end this is just my opinion and with my assumptions. I would be very helpful to her the opinion of the maintainers and others as well.

Edit: Here this was discussed for the CLI and rejected: #470

@hick209
Copy link
Contributor

hick209 commented Aug 7, 2024

This is failing the spotless check (just formatting apparently).
Would you mind pushing changes to fix that, @robinpokorny ?

I apologize the response delay. I was busy shipping Kotlin 2.0 internally

NumberFormat format = NumberFormat.getInstance();
NumberFormatter formatter = new NumberFormatter(format);
formatter.setValueClass(Integer.class);
formatter.setMinimum(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's bold! Not big deal, but how about making this something like 10 or at the very least 1?

@rock3r
Copy link
Contributor

rock3r commented Aug 8, 2024

After #502 is merged, this PR will get severely out of sync with the main branch as everything is converted to Kotlin (and the UI to the Kotlin UI DSL). I was planning to expose this and the other parameters as a "custom" option in the dropdown, revealing additional UI, in a follow up. I just noticed that there was a PR open for part of it... and I don't want to look like I'm taking someone else's idea :) Happy to leave that to you, if you're ok with doing the rest of what I was planning too, or to do everything myself — likely over the weekend, or next week.

@simonhauck
Copy link

@hick209 Did you discuss internally if these options should be exposed? And if yes, can we add them also to the CLI?

@hick209
Copy link
Contributor

hick209 commented Aug 13, 2024

We have discussed the idea internally and have come to the conclusion that we do not want to expose this option via IntelliJ plugin.
The reason for this is the same reason as to why the CLI is not configurable, but Gradle/maven plugins are, which is share-ability and also this principle. Let me try to explain the share-ability part.

One of the goals of ktfmt (and also most formatters) is to reduce bikeshedding discussions related to code formatting.
We assume code is shared among developers and for that end we only allow configurations that could be shared as well.

It is an intentional decision to allow for Gradle and Maven plugins, since those are usually checked in files that are part of the codebase.
This means there shouldn't be a conflict where one person is formatting a file with 100 line width and another with 120 line width, since a change like that would have to touch a file which would have to be committed to the repository and for that reviewed.

From our experience IntelliJ configuration sharing is not easily done within an organization/codebase and for that reason we do not intent to allow the current IJ plugin to be more configuration than the CLI tool, which follows the same principle.

I'll try to update the README to reflect the information shared here. Thank you so much for probing us regarding this matter and causing the improvement of our documentation.

@hick209 hick209 closed this Aug 13, 2024
@hick209
Copy link
Contributor

hick209 commented Aug 13, 2024

@rock3r actually shared with us that this could be shared with the file .idea/ktfmt.xml file, therefore we should be good to accept the changes. I'll take #503 though as that one is more complete.

@robinpokorny
Copy link
Author

@hick209, thanks for the explanation.

I wholeheartedly agree with reducing the bikeshedding. On the other hand, I'm not convinced that Gradle and Maven plugins should have a special treatment. The main reason for my PR was that using the official ktfmt editor plugin could not be configured in a compatible way with CI checks using Gradle. This made the plugin not only unusable, but it IMO removes the benefit of using a strict common formatter.

I'd say the proper solution would either disallow those options completely, or store them in a usage-independent way (for example, like .editorconfig or .prettierrc).

In the end, I'm happy there is now a way to configure the plugin!

@rock3r
Copy link
Contributor

rock3r commented Aug 14, 2024

It was the driving force behind my pr as well! Hopefully they come up with a universal settings mechanism that's supported by all ktfmt incarnations, building on top of what's here now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants