SONARPY-749 Add new style importing of Pylint reports by guillaume-dequenne · Pull Request #808 · SonarSource/sonar-python

guillaume-dequenne · 2020-07-07T14:09:15Z

No description provided.

gyula-sallai-sonarsource

Mostly minor comments on the implementation, I will take another look at the tests tomorrow.

gyula-sallai-sonarsource · 2020-07-08T15:27:47Z

+  private static final String RULES_JSON = "org/sonar/plugins/python/pylint/rules.json";
+
+  private static final ExternalRuleLoader RULE_LOADER = new ExternalRuleLoader(PylintSensor.LINTER_KEY, PylintSensor.LINTER_NAME, RULES_JSON, Python.KEY);
+


nitpick I would drop this extra empty line.

gyula-sallai-sonarsource · 2020-07-08T15:28:49Z

+  private static final Pattern PATTERN_LEGACY = Pattern.compile("(.+):(\\d+): \\[(.*)\\] (.*)");
+
+  protected Issue parseLine(String line) {
+


nitpick I would drop this empty line.

gyula-sallai-sonarsource · 2020-07-08T15:31:33Z

+  private static final Pattern PYLINT_DEFAULT = Pattern.compile("(.+):(\\d+):(\\d+): (\\S+): (.*)");
+  private static final Pattern PATTERN_LEGACY = Pattern.compile("(.+):(\\d+): \\[(.*)\\] (.*)");


The naming is a little inconsistent for me, I would prefer something like DEFAULT_PATTERN and LEGACY_PATTERN, or PYLINT_DEFAULT and PYLINT_LEGACY.

gyula-sallai-sonarsource · 2020-07-08T15:35:36Z

+        String message = m.group(5);
+        return new Issue(filePath, ruleKey, message, lineNumber, columnNumber);
      }
+      m = PYLINT_PATTERN.matcher(line);


I would add a comment here that justifies why are we using a Pylint pattern here (maybe also worth explaining that even though we have the same pattern in PylintReportReader, the implementation is slightly different).

After having a second look at this, the different formats are only subtly different and I decided to simply make a more generic version of the regexes and logic in TextReportReader rather than keep the 2 implementations.

gyula-sallai-sonarsource · 2020-07-08T15:37:31Z

+    logger().error("No issues information will be saved as the report file '{}' can't be read. " +
+      e.getClass().getSimpleName() + ": " + e.getMessage(), reportPath, e);


It's a little weird to me that we are mixing string formatting and string concatenation here.

gyula-sallai-sonarsource · 2020-07-09T07:06:17Z

+    } catch (IOException e) {
+      throw new IllegalStateException(e);
+    }


Not sure I understand why we are doing this, executeSensorImporting has already declared that it may throw IOException

After double checking it seems that it's because this method is called in a forEach statement, and because of that the IOException it might throw would not be handled by the executeSensorImporting throw declaration.

gyula-sallai-sonarsource

LGTM, left some subjective comments. Feel free to disregard both of them if you disagree.

gyula-sallai-sonarsource · 2020-07-09T11:29:26Z

+ */
+public class TextReportReader {
+
+  private final int reportOffset;


nitpick I would add an empty line here just to indicate that this is an instance field. Maybe also would move it below the static fields.

gyula-sallai-sonarsource · 2020-07-09T11:34:24Z

-  private static final Logger LOG = Loggers.get(Flake8ReportReader.class);
-  private static final Pattern DEFAULT_PATTERN = Pattern.compile("(.+):(\\d+):(\\d+): (\\S+) (.*)");
-  private static final Pattern PYLINT_PATTERN = Pattern.compile("(.+):(\\d+): \\[(.*)\\] (.*)");
+  public TextReportReader(int columnStartIndex) {


minor, subjective Having to call this constructor with some magic numbers seems a little off to me. I think it would look nicer if we introduced integer constants and used them something like this:

new TextReportReader(TextReportReader.COLUMN_ZERO_BASED) new TextReportReader(TextReportReader.COLUMN_ONE_BASED)

Feel free to disregard if you feel that this is too much.

sonarsource-next · 2020-07-09T12:07:27Z

Kudos, SonarQube Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots (100.0% reviewed)
0 Code Smells

100.0% Coverage
0.0% Duplication

GitOrigin-RevId: 1b88d9f35afcb9554ab9357c4c6cdbdc986e7454

guillaume-dequenne force-pushed the SONARPY-749 branch 6 times, most recently from 988bbf6 to 4d1aa19 Compare July 8, 2020 12:17

guillaume-dequenne changed the title ~~temp commit new pylint~~ SONARPY-749 Add new style importing of Pylint reports Jul 8, 2020

guillaume-dequenne force-pushed the SONARPY-749 branch 2 times, most recently from 595bd54 to 7dbd8df Compare July 8, 2020 14:53

guillaume-dequenne added 5 commits July 8, 2020 17:15

Add new style importing of Pylint reports

5f9c621

Mutualize Flake8 and Pylint report readers

caac6db

Mutualize saving of Pylint and Flake8 issues

f32212a

Mutualize issue logging for unreadable file

1ba784d

Remove debug log whitelines report reader

1132d03

guillaume-dequenne force-pushed the SONARPY-749 branch from 7dbd8df to 1132d03 Compare July 8, 2020 15:16

guillaume-dequenne requested a review from gyula-sallai-sonarsource July 8, 2020 15:16

gyula-sallai-sonarsource suggested changes Jul 8, 2020

View reviewed changes

gyula-sallai-sonarsource reviewed Jul 9, 2020

View reviewed changes

guillaume-dequenne force-pushed the SONARPY-749 branch from ab3802f to f62318c Compare July 9, 2020 09:04

Fixes after review

380f8f2

guillaume-dequenne force-pushed the SONARPY-749 branch from f62318c to 380f8f2 Compare July 9, 2020 09:09

guillaume-dequenne requested a review from gyula-sallai-sonarsource July 9, 2020 09:39

gyula-sallai-sonarsource approved these changes Jul 9, 2020

View reviewed changes

Fix magic numbers

fa01f66

guillaume-dequenne force-pushed the SONARPY-749 branch from 25b12a9 to fa01f66 Compare July 9, 2020 12:02

guillaume-dequenne merged this pull request into MMF-1778 Jul 9, 2020

guillaume-dequenne deleted the SONARPY-749 branch July 9, 2020 12:22

guillaume-dequenne added a commit that referenced this pull request Jul 9, 2020

SONARPY-749 Add new style importing of Pylint reports (#808)

516d81c

guillaume-dequenne added a commit that referenced this pull request Jul 14, 2020

SONARPY-749 Add new style importing of Pylint reports (#808)

877c7c7

guillaume-dequenne added a commit that referenced this pull request Jul 14, 2020

SONARPY-749 Add new style importing of Pylint reports (#808)

30eeb2e

hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request Jan 28, 2026

SONARPY-3700 Fix typeshed-stub-manager frontmatter (#808)

0c93414

GitOrigin-RevId: 1b88d9f35afcb9554ab9357c4c6cdbdc986e7454

		private static final String RULES_JSON = "org/sonar/plugins/python/pylint/rules.json";

		private static final ExternalRuleLoader RULE_LOADER = new ExternalRuleLoader(PylintSensor.LINTER_KEY, PylintSensor.LINTER_NAME, RULES_JSON, Python.KEY);

		private static final Pattern PATTERN_LEGACY = Pattern.compile("(.+):(\\d+): \\[(.)\\] (.)");

		protected Issue parseLine(String line) {

		private static final Pattern PYLINT_DEFAULT = Pattern.compile("(.+):(\\d+):(\\d+): (\\S+): (.*)");
		private static final Pattern PATTERN_LEGACY = Pattern.compile("(.+):(\\d+): \\[(.)\\] (.)");

		logger().error("No issues information will be saved as the report file '{}' can't be read. " +
		e.getClass().getSimpleName() + ": " + e.getMessage(), reportPath, e);

Conversation

guillaume-dequenne commented Jul 7, 2020

Uh oh!

gyula-sallai-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyula-sallai-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarsource-next Bot commented Jul 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants