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.

Sign up for GitHub

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] resolve SonarCloud violations #221

Closed
wants to merge 1 commit into from
Closed

[WIP] resolve SonarCloud violations #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,6 @@
<artifactId>cobertura-maven-plugin</artifactId>
<version>2.7</version>
<configuration>
<!--<quiet>true</quiet>-->
<formats>
<format>xml</format>
<format>html</format>
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/sonar/plugins/checkstyle/CheckstyleAuditListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public void addError(AuditEvent event) {
final NewIssue issue = context.newIssue();
final ActiveRule rule = ruleFinder.find(
RuleKey.of(CheckstyleConstants.REPOSITORY_KEY, ruleKey));
if (Objects.nonNull(issue) && Objects.nonNull(rule)) {
if (Objects.nonNull(issue) && Objects.nonNull(rule) && Objects.nonNull(message)) {
final NewIssueLocation location = issue.newLocation()
.on(currentResource)
.at(currentResource.selectLine(getLineId(event)))
Expand Down
8 changes: 5 additions & 3 deletions src/main/java/org/sonar/plugins/checkstyle/CheckstyleExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.Objects;
import java.util.stream.Collectors;

import com.puppycrawl.tools.checkstyle.api.AutomaticBean;
import org.apache.commons.io.FileUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -51,7 +52,7 @@
@ExtensionPoint
@ScannerSide
public class CheckstyleExecutor {
public static final String PROPERTIES_PATH =
public static final String PROPERTIES_LOCATION =
"/org/sonar/plugins/checkstyle/checkstyle-plugin.properties";

private static final Logger LOG = LoggerFactory.getLogger(CheckstyleExecutor.class);
Expand Down Expand Up @@ -103,15 +104,16 @@ private void executeWithClassLoader(URLClassLoader projectClassloader) {
if (xmlReport != null) {
LOG.info("Checkstyle output report: {}", xmlReport.getAbsolutePath());
xmlOutput = FileUtils.openOutputStream(xmlReport);
checker.addListener(new XMLLogger(xmlOutput, true));
checker.addListener(new XMLLogger(xmlOutput, AutomaticBean.OutputStreamOptions.CLOSE));
}

checker.setCharset(configuration.getCharset().name());
checker.configure(configuration.getCheckstyleConfiguration());
checker.process(configuration
.getSourceFiles()
.stream()
.map(InputFile::file)
.map(InputFile::uri)
.map(File::new)
.collect(Collectors.toList()));
}
catch (Exception ex) {
Expand Down
23 changes: 12 additions & 11 deletions src/main/java/org/sonar/plugins/checkstyle/CheckstylePlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@
import java.util.Arrays;
import java.util.List;

import org.sonar.api.CoreProperties;
import org.sonar.api.Plugin;
import org.sonar.api.PropertyType;
import org.sonar.api.SonarPlugin;
import org.sonar.api.config.PropertyDefinition;
import org.sonar.api.resources.Qualifiers;

public final class CheckstylePlugin extends SonarPlugin {
public final class CheckstylePlugin implements Plugin {

private static final String CHECKSTYLE_SUB_CATEGORY_NAME = "Checkstyle";

Expand Down Expand Up @@ -67,44 +66,46 @@ public final class CheckstylePlugin extends SonarPlugin {
+ "Checkstyle</a> "
+ "configuration for more information.";

@SuppressWarnings("rawtypes")
@Override
public List getExtensions() {
public void define(Context context) {
context.addExtensions(getCheckstyleExtensions());
}

@SuppressWarnings("rawtypes")
public List getCheckstyleExtensions() {
return Arrays
.asList(PropertyDefinition.builder(CheckstyleConstants.CHECKER_FILTERS_KEY)
.defaultValue(CheckstyleConstants.CHECKER_FILTERS_DEFAULT_VALUE)
.category(CoreProperties.CATEGORY_JAVA)
.category(CheckstyleConstants.JAVA_KEY)
.subCategory(CHECKSTYLE_SUB_CATEGORY_NAME)
.name("Checker Filters")
.description(CHECKER_FILTERS_DESCRIPTION)
.type(PropertyType.TEXT)
.onQualifiers(Qualifiers.PROJECT, Qualifiers.MODULE).build(),
PropertyDefinition.builder(CheckstyleConstants.TREEWALKER_FILTERS_KEY)
.defaultValue(CheckstyleConstants.TREEWALKER_FILTERS_DEFAULT_VALUE)
.category(CoreProperties.CATEGORY_JAVA)
.category(CheckstyleConstants.JAVA_KEY)
.subCategory(CHECKSTYLE_SUB_CATEGORY_NAME)
.name("Treewalker Filters")
.description(TREEWALKER_FILTERS_DESCRIPTION)
.type(PropertyType.TEXT)
.onQualifiers(Qualifiers.PROJECT, Qualifiers.MODULE).build(),
PropertyDefinition.builder(CheckstyleConstants.CHECKER_TAB_WIDTH)
.category(CoreProperties.CATEGORY_JAVA)
.category(CheckstyleConstants.JAVA_KEY)
.subCategory(CHECKSTYLE_SUB_CATEGORY_NAME)
.name("Tab Width")
.description(CHECKER_TAB_WIDTH_DESCRIPTION)
.type(PropertyType.INTEGER)
.onQualifiers(Qualifiers.PROJECT, Qualifiers.MODULE)
.build(),
PropertyDefinition.builder(CheckstyleConfiguration.PROPERTY_GENERATE_XML)
.defaultValue("false").category(CoreProperties.CATEGORY_JAVA)
.defaultValue("false").category(CheckstyleConstants.JAVA_KEY)
.subCategory(CHECKSTYLE_SUB_CATEGORY_NAME)
.name("Generate XML Report").type(PropertyType.BOOLEAN).hidden()
.build(),

CheckstyleSensor.class, CheckstyleConfiguration.class,
CheckstyleExecutor.class, CheckstyleAuditListener.class,
CheckstyleProfileExporter.class, CheckstyleProfileImporter.class,
CheckstyleRulesDefinition.class);
}

}
2 changes: 1 addition & 1 deletion src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileExporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ private void appendCustomFilters(Writer writer) throws IOException {

private void appendTabWidth(Writer writer) throws IOException {
final String tabWidth = configuration.get(CheckstyleConstants.CHECKER_TAB_WIDTH)
.orElse(null);
.orElse("");
appendModuleProperty(writer, "tabWidth", tabWidth);
}

Expand Down
22 changes: 20 additions & 2 deletions src/main/java/org/sonar/plugins/checkstyle/CheckstyleRulesDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,16 @@

import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.util.Objects;
import java.util.Properties;

import com.google.common.base.Charsets;
import com.google.common.io.Resources;
import org.sonar.api.ExtensionPoint;
import org.sonar.api.batch.ScannerSide;
import org.sonar.api.server.rule.RulesDefinition;
import org.sonar.api.server.rule.RulesDefinitionXmlLoader;
import org.sonar.squidbridge.rules.ExternalDescriptionLoader;
import org.sonar.squidbridge.rules.SqaleXmlLoader;

import com.google.common.annotations.VisibleForTesting;
Expand Down Expand Up @@ -61,14 +63,30 @@ static void extractRulesData(NewRepository repository, String xmlRulesFilePath,
.getResourceAsStream(xmlRulesFilePath)) {
ruleLoader.load(repository, resource, "UTF-8");
}
ExternalDescriptionLoader.loadHtmlDescriptions(repository, htmlDescriptionFolder);
loadHtmlDescriptions(repository, htmlDescriptionFolder);
try (InputStream resource = CheckstyleRulesDefinition.class
.getResourceAsStream("/org/sonar/l10n/checkstyle.properties")) {
loadNames(repository, resource);
}
SqaleXmlLoader.load(repository, "/com/sonar/sqale/checkstyle-model.xml");
}

private static void loadHtmlDescriptions(NewRepository repository, String htmlDescriptionFolder) {
// code adapted from:
// https://github.com/SonarSource/sslr-squid-bridge/blob/2.7.0.377/
// src/main/java/org/sonar/squidbridge/rules/ExternalDescriptionLoader.java
Copy link
Member

Choose a reason for hiding this comment

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

Can we move these comments to the Javadoc?
Maybe copy the first sentence from sonar's javadoc (if there is one) and append a note saying it was copied from and use {@link ExternalDescriptionLoader#loadHtmlDescriptions(NewRepository, String)} instead of url to github.

Copy link
Contributor Author

@muhlba91 muhlba91 May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we can but shouldn't we also move https://github.com/checkstyle/sonar-checkstyle/blob/master/src/main/java/org/sonar/plugins/checkstyle/CheckstyleRulesDefinition.java#L73 to the Javadoc as it follows the same?

However, I don't quite understand what you mean with {@link ExternalDescriptionLoader#loadHtmlDescriptions(NewRepository, String)}?
Because for now, this class still exists, just like loadNames (see link before) still existed until they removed some of their deprecated code. I assume this will happen here as well, meaning ExternalDescriptionLoader won't exist at some point anymore.

Both methods don't have any Javadoc in sslr-squid-bridge.

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we also move ... to the Javadoc as it follows the same?

Yes I like that.

Because for now, this class still exists, just like loadNames (see link before) still existed until they removed some of their deprecated code. I assume this will happen here as well, meaning ExternalDescriptionLoader won't exist at some point anymore.

Ok, URL has tag in it so it will never go bad unless github goes away.
{@link was just a javadoc tag that would take user to that method if they clicked on it like in an IDE. If class/method does go away, javadoc tool should print an error on the {@link as it won't point to a valid class.

I was mainly just pointing its usage since I feel it is a better way to point to a class/method in a javadoc. If everyone wants to stay with URL, I am fine.

Copy link
Member

Choose a reason for hiding this comment

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

@muhlba91 , it this discussion point resolved ?
If yes, please pin @rnveach to approve PR.

Copy link
Member

Choose a reason for hiding this comment

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

It has not been resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this point I was waiting for a decision if we want to put dead Javadoc links in or stick with GitHub URL references?
I'm good with both ways but we should still keep the URL somewhere as well for a full reference to where the code was taken from (incl. sslr-squid-bridge version). Otherwise, you won't really know from where/when the code was taken over and whether it was modified by us already or not.

Copy link
Member

Choose a reason for hiding this comment

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

I see no reason in reference of javadoc in other project, especially to non-existing code (non-existing javadoc).

Please move link from single comment to loadHtmlDescriptions javadoc styles comment. Please do the same in "loadNames". Simply move whole text of comment to javadoc (no extra descriptions are required.)

for (NewRule rule : repository.rules()) {
final URL resource = CheckstyleRulesDefinition.class.getResource(htmlDescriptionFolder
+ "/" + rule.key() + ".html");
try {
rule.setHtmlDescription(Resources.toString(resource, Charsets.UTF_8));
}
catch(IOException e) {
throw new IllegalStateException("Failed to read: " + resource, e);
}
}
}

private static void loadNames(NewRepository repository, InputStream stream) {
// code taken from:
// https://github.com/SonarSource/sslr-squid-bridge/blob/2.5.2/
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/sonar/plugins/checkstyle/CheckstyleConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public void writeConfigurationToWorkingDir() throws IOException {
public void getCheckstyleConfiguration() throws Exception {
fileSystem.setEncoding(StandardCharsets.UTF_8);
final MapSettings mapSettings = new MapSettings(new PropertyDefinitions(
new CheckstylePlugin().getExtensions()));
new CheckstylePlugin().getCheckstyleExtensions()));
mapSettings.setProperty(CheckstyleConstants.CHECKER_FILTERS_KEY,
CheckstyleConstants.CHECKER_FILTERS_DEFAULT_VALUE);
final org.sonar.api.config.Configuration settings = new ConfigurationBridge(mapSettings);
Expand Down
8 changes: 7 additions & 1 deletion src/test/java/org/sonar/plugins/checkstyle/CheckstylePluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,19 @@
import static org.fest.assertions.Assertions.assertThat;

import org.junit.Test;
import org.mockito.Mockito;
import org.sonar.api.Plugin;
import org.sonar.api.SonarRuntime;

public class CheckstylePluginTest {

@Test
public void testGetExtensions() {
final SonarRuntime sonarRuntime = Mockito.mock(SonarRuntime.class);
final CheckstylePlugin plugin = new CheckstylePlugin();
assertThat(plugin.getExtensions().size()).isGreaterThan(1);
final Plugin.Context context = new Plugin.Context(sonarRuntime);
plugin.define(context);
assertThat(context.getExtensions().size()).isGreaterThan(1);
}

}
2 changes: 1 addition & 1 deletion src/test/java/org/sonar/plugins/checkstyle/CheckstyleProfileExporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ public void addTabWidthProperty() {
@SuppressWarnings("unchecked")
private void initSettings(@Nullable String key, @Nullable String property) {
final MapSettings mapSettings = new MapSettings(
new PropertyDefinitions(new CheckstylePlugin().getExtensions()));
new PropertyDefinitions(new CheckstylePlugin().getCheckstyleExtensions()));
if (Objects.nonNull(key)) {
mapSettings.setProperty(key, property);
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/sonar/plugins/checkstyle/CheckstyleVersionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public class CheckstyleVersionTest {

@Test
public void getCheckstyleVersion() {
final String version = CheckstyleExecutor.PROPERTIES_PATH;
final String version = CheckstyleExecutor.PROPERTIES_LOCATION;
assertThat(new CheckstyleVersion().getVersion(version).length()).isGreaterThan(1);
}

Expand Down