Skip to content

MMF-1778#806

Merged
guillaume-dequenne merged 5 commits intomasterfrom
MMF-1778
Jul 14, 2020
Merged

MMF-1778#806
guillaume-dequenne merged 5 commits intomasterfrom
MMF-1778

Conversation

@guillaume-dequenne
Copy link
Copy Markdown
Contributor

@guillaume-dequenne guillaume-dequenne commented Jul 6, 2020

No description provided.

@guillaume-dequenne guillaume-dequenne changed the title Prepare for next development iteration MMF-1778 Jul 7, 2020
pynicolas
pynicolas reviewed Jul 13, 2020
View reviewed changes
Comment thread sonar-python-plugin/src/main/java/org/sonar/plugins/python/pylint/PylintSensor.java
Comment on lines -79 to -80
LOG.warn("Execution of Pylint is deprecated and will be removed." +
" Instead, Pylint should be executed before sonar-scanner and its report should be imported using the '" + PylintImportSensor.REPORT_PATH_KEY + "' property.");
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

What happens if a user was still relying on pylint execution mode? The user would lose issues and would maybe not understand why (maybe his/her SonarQube administrator updated sonar-python-plugin without notice).

Should we log a warning when we detect that some properties like sonar.python.pylint or sonar.python.pylint_config are set? That's just an idea and it's far from perfect: as such properties had default values, there may be cases where people use pylint execution mode without setting those properties.

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Jul 14, 2020

Choose a reason for hiding this comment

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

After discussion with Alex, we agreed that it's not needed as we have displayed a deprecation warning for quite some time now.

Comment thread sonar-python-plugin/src/main/java/org/sonar/plugins/python/PythonPlugin.java
.onQualifiers(Qualifiers.PROJECT)
.multiValues(true)
.build(),
PylintRulesDefinition.class);
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

Very minor: it's weird to have PylintRulesDefinition here and PylintSensor in a separate call to context.addExtension.

Comment thread sonar-python-plugin/src/main/java/org/sonar/plugins/python/TextReportReader.java Outdated
Comment on lines +39 to +40
protected static final Pattern DEFAULT_PATTERN = Pattern.compile("(.+):(\\d+):(\\d+): (\\S+[^:]):? (.*)");
protected static final Pattern LEGACY_PATTERN = Pattern.compile("(.+):(\\d+): \\[(.*)\\] (.*)");
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

Why protected?

Comment thread sonar-python-plugin/src/main/java/org/sonar/plugins/python/TextReportReader.java Outdated
return null;
}

private static Issue extractDefaultStyleIssue(Matcher m, int columnOffset) {
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

Do we need to have columnOffset as a parameter? Can't we read this.reportOffset?

Comment thread sonar-python-plugin/src/main/java/org/sonar/plugins/python/bandit/BanditSensor.java
protected void importReport(File reportPath, SensorContext context, Set<String> unresolvedInputFiles) throws IOException, ParseException {
InputStream in = new FileInputStream(reportPath);
LOG.info("Importing {}", reportPath);
boolean engineIdIsSupported = context.getSonarQubeVersion().isGreaterThanOrEqual(Version.create(7, 4));
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

Do we still need to support SQ 7.4? Maybe we can change that in a separate PR.

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Jul 14, 2020

Choose a reason for hiding this comment

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

Indeed, I had in mind to have a small clean-up PR afterwards to freshen a bit BanditSensor among other things, didn't want to mix that with that with the actual implementation for ease of reviewing.

Comment thread sonar-python-plugin/src/main/resources/org/sonar/plugins/python/pylint/rules.json Outdated
"key": "C0111",
"name": "Missing docstring",
"priority": "MINOR",
"status": "DEPRECATED"
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

I think we shouldn't set this deprecation status.
As far as I know, the original motivation was to reimplement pylint rules and to mark the pylint rules we had reimplemented as deprecated. I don't think it makes sense in the context of external issues.

Comment thread sonar-python-plugin/src/main/java/org/sonar/plugins/python/TextReportReader.java Outdated
String filePath = m.group(1);
int lineNumber = Integer.parseInt(m.group(2));
String ruleKey = m.group(3);
int lastIndex = Math.min(ruleKey.indexOf(","), ruleKey.indexOf("("));
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

Do we need ruleKey.indexOf(",")? Do we have a test case to cover it?

Copy link
Copy Markdown
Contributor Author

@guillaume-dequenne guillaume-dequenne Jul 14, 2020

Choose a reason for hiding this comment

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

I thought I saw such format somewhere, but impossible to find it again...I'll remove this.

Comment thread sonar-python-plugin/src/test/java/org/sonar/plugins/python/pylint/PylintSensorTest.java
@Test
public void no_issues_unknown_files() throws IOException {
List<ExternalIssue> externalIssues = executeSensorImporting(7, 9, "pylint_report_unknown_files.txt");
assertThat(externalIssues).isEmpty();
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

Should we assert anything about the logs?

Comment thread sonar-python-plugin/src/main/java/org/sonar/plugins/python/ExternalIssuesSensor.java Outdated
Comment on lines +58 to +60
// Only for PylintSensor
return reportPathKey().equals(PylintSensor.REPORT_PATH_KEY) &&
conf.hasKey(PYLINT_LEGACY_KEY);
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Jul 13, 2020

Choose a reason for hiding this comment

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

Alternative solution: rely on polymorphism and define this condition in PylintSensor.

@guillaume-dequenne guillaume-dequenne force-pushed the MMF-1778 branch 2 times, most recently from 16f5e7f to 3083c9a Compare July 14, 2020 13:29
@sonarsource-next
Copy link
Copy Markdown

sonarsource-next Bot commented Jul 14, 2020

Kudos, SonarQube Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots (100.0% reviewed)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
1.2% 1.2% Duplication

@guillaume-dequenne guillaume-dequenne marked this pull request as ready for review July 14, 2020 15:07
@guillaume-dequenne guillaume-dequenne merged commit fe0772d into master Jul 14, 2020
@guillaume-dequenne guillaume-dequenne deleted the MMF-1778 branch July 14, 2020 15:17
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request Feb 10, 2026
@Seppli11
SONARPY-3699 Add CLI tool for agents (#806)
516cedc 
GitOrigin-RevId: 2136b155a91f9acf77c7ef0b3d9fbda7d5fee260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@pynicolas pynicolas pynicolas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@guillaume-dequenne @pynicolas