Remove transitive ktfmt dependencies to prevent kotlin version compatibility issues

[simonhauck]

🚀 Description

Hey @cortinico ,

I investigated a bit more the Kotlin update task and wanted to get your opinion before continuing.

I found two ways to make it compile:

Either exclude all transitive dependencies or just exclude the Kotlin stuff as shown below.

    testImplementation(libs.ktfmt){
//        isTransitive = false
        exclude(group = "org.jetbrains.kotlin")
    }

I checked the code and unfortunately at one place we use something from a transitive dependency. This is the FormatterException to get the error message. Fun fact, I think the code catched the wrong error? (The ktfmt code annotation its method with the FormatterException while this plugins catched the FormattingError).

So if we remove this peace of code, that might not have worked anyway (at least to my knowledge, please correct me if i am wrong), then we could use isTransitive=false and would remove all transitive dependencies. If we want to keep the behavior, we can just exclude the Kotlin lib.

When we go either of these ways we have to change / move the behavior of the KtfmtFormatterTest class. This invokes right now directly kftmt, which obviously fails if we exclude some dependencies. I am fine with that, since we can check/test the behavior over the gradle tasks, but I wanted to get your opinion.

So @cortinico I would like to know, if you are okey with excluding the dependencies and what way your prefer ?

📄 Motivation and Context

🧪 How Has This Been Tested?

📦 Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

✅ Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[cortinico]

When we go either of these ways we have to change / move the behavior of the KtfmtFormatterTest class. This invokes right now directly kftmt, which obviously fails if we exclude some dependencies. I am fine with that, since we can check/test the behavior over the gradle tasks, but I wanted to get your opinion.

I'd say, it's safer to exclude just Kotlin. The reason is: if ktfmt adds another 3p dependency, we might fail to build/run because of that.

Regarding the snippet you mentioned: I think it's safe to clean it up OR we could re-declare the com.google.googlejavaformat in our dependencies, but as you said, I think that part never worked

[simonhauck]

Hey @cortinico ,

I spent some time on it, and the small change went into a small to medium refactor.

The main thing is:

• Removing the transitive dependency to ktfmt.

Which lead to the following additional changes:

• That required a small rework of the exception handling as mentioned above. In the end, my proposal is to just use the exception message directly. This gives often some additional information and from my point of view offers even a benefit.

• I wanted to collect all logic in a single class and found the inheritance we had before sometimes hard to understand. So I tried to simplify the call chain by removing the inheritance in the KtfmtWorker and reduced it a bit in the KtfmtCheck and KtfmtFormat Task

• I wanted to align as good as possible the check and format task. Thats why some logic went to the base class. Both tasks have now an output file that writes the summary. I think nobody is really using this file, so the changed layout should not be a problem.

• Some log messages are now slightly different

• Since I changed some methods, I had to update the api file. But this should only be internal stuff and not effect any users.

• In the last commit i moved the toFormattingOption method inside the KtfmtWorkAction. Now, really only this class has acess to facebook.ktfmt and we can remove the dependency from the test completly (which is very nice in my oppinion), but for that to work I had to remove also the test. I think this method has a pretty low risk and some stuff is tested via the different formatting options. But we can revert this commit if you prefer.

• Last but not least, i could simplify the kolin version compatibility test, and removed the mavenLocal stuff :)

I know it got a bit larger then anticipated, but at least the Integration tests did more or less work :D Only some minor changes to the log messages. This is my Proposal. Could you have a look and tell me your oppinion?

[cortinico]

I know it got a bit larger then anticipated, but at least the Integration tests did more or less work :D Only some minor changes to the log messages. This is my Proposal. Could you have a look and tell me your oppinion?

In general I like the direction, but the code is harder to review since is a 1K LOC of changes.
Perhaps we could split it in 2/3 PRs if it's not too much work?