-
Notifications
You must be signed in to change notification settings - Fork 70
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
Issue #40: removed sonar-java-plugin #439
Conversation
<artifactId>sonar-java-plugin</artifactId> | ||
<version>${sonar-java.version}</version> | ||
<groupId>com.fasterxml.staxmate</groupId> | ||
<artifactId>staxmate</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New dependency was required since the other included it and we still use it as well.
@@ -34,7 +33,7 @@ public CheckstyleSensor(CheckstyleExecutor executor) { | |||
|
|||
@Override | |||
public void describe(SensorDescriptor descriptor) { | |||
descriptor.onlyOnLanguage(Java.KEY).name("CheckstyleSensor"); | |||
descriptor.onlyOnLanguage("java").name("CheckstyleSensor"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from sonar-java-plugin
and so we have to hardcode it since we want to remove that dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public class Java extends AbstractLanguage {
/**
* Java key
*/
public static final String KEY = "java";
Kudos, SonarCloud Quality Gate passed! |
How we can prove that plugin continue to work? compilation is not enough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@romani I do not know how to test this. |
@muhlba91 usually runs it during release, so I hope we will do this that time also so it will be tested |
i usually test it e2e when opening a MR for a new version upgrade, and if other changes happen between that merge and the release i also test it again before releasing it. i‘ll run a deeper regression test than usually after the changes being merged now. fyi: next release will take a few more days (probably ~1.5 weeks) as i‘m on business travels since a few weeks and quite busy. i‘ll then shortly after perform the next version upgrade. |
Issue #40
Removed the dependency.
I was not seeing any classpath shenanigans mentioned in the issue unless its happening via reflection or such.
The code before this was causing issues with Eclipse:
https://stackoverflow.com/questions/55571046/eclipse-is-confused-by-imports-accessible-from-more-than-one-module