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

[WIP] Replace "kotlin-compiler-embeddable" by "kotlin-compiler" #424

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leveretka
Copy link
Contributor

@leveretka leveretka commented Jun 8, 2024

I only migrated our plugin, however the module "utils-kotlin" still needs "kotlin-compiler-embeddable", so had to copy few classes to avoid dependency clashes.

Comment on lines +5 to +6
import com.google.common.collect.ImmutableList // Noncompliant
import com.google.common.collect.ImmutableList.copyOf // Noncompliant
Copy link
Contributor Author

@leveretka leveretka Jun 11, 2024

Choose a reason for hiding this comment

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

This is the only issue affected, but those 2 are TP. But only on Linux/MacOS (the Windows build is failing because of this change). I don't know what to do since this is a partial semantic test. Maybe we can accept this or drop these lines.

@leveretka leveretka force-pushed the margo/migrate-kotlin-compiler branch from f47b6a3 to 5883e12 Compare June 11, 2024 13:54
@leveretka leveretka force-pushed the margo/migrate-kotlin-compiler branch from 5883e12 to e860acf Compare June 11, 2024 14:00
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
36.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good overall, let's make sure we document some of the changes namely in the non-transitive dependency import in our build.gradle.kts files.

@@ -96,7 +96,7 @@ private static void parseFiles(List<File> reports, UnitTestIndex index) {
try {
parser.parse(report);
} catch (XMLStreamException e) {
throw new ParseException("Fail to parse the Surefire report: " + report, null, e);
throw new ParseException(e);

Choose a reason for hiding this comment

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

I see that the constructor of the class does not take a message AND a cause. Can we maybe log it a least so that we do not completely lose the information?

Suggested change
throw new ParseException(e);
LOGGER.warn("Fail to parse the Surefire report: " + report);
throw new ParseException(e);

@@ -15,8 +15,12 @@ dependencies {
implementation(libs.kotlin.compiler.embeddable)
implementation(utilLibs.bundles.ktlint)

implementation(project(":sonar-kotlin-plugin"))
implementation(project(":sonar-kotlin-api"))
implementation(project(":sonar-kotlin-plugin")) {

Choose a reason for hiding this comment

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

Can we leave a comment here explaining why we place this restriction on inheriting the transitive dependencies of the project? WIthout a ticket, it will be difficult in the future to understand why me made this change

Choose a reason for hiding this comment

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

I am guessing this is the class that was copied. Let's add a comment with the origin of this file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants