Update the plugin api #7777

costin-zaharia-sonarsource · 2023-08-10T15:30:19Z

In order to be able to update the RSPEC for clean code taxonomy.

Update of sonar-analyzer-commons

Release notes of sonar-analyzer-commons 2.6.0.1473 and 2.7.0.1482:

Avoid calling 'scan' two times in code recognizers by @Wohops in Avoid calling 'scan' two times in code recognizers sonar-analyzer-commons#297
Add keySet method on PMap and stream method on PSet and PStack by @Swalkyn in Add keySet method on PMap and stream method on PSet and PStack sonar-analyzer-commons#298
Update sonar-plugin-api to 10.0 and use slf4j Logger by @alban-auzeill in Update sonar-plugin-api to 10.0 and use slf4j Logger sonar-analyzer-commons#300
Support Clean Code Taxonomy data by @irina-batinic-sonarsource in Support Clean Code Taxonomy data sonar-analyzer-commons#303
Prevent ExternalRuleLoader to manipulate code attribute and impact fields when runtime API < 10.1 by @alban-auzeill in Prevent ExternalRuleLoader to manipulate code attribute and impact fields when runtime API < 10.1 sonar-analyzer-commons#305
Remove usage of @beta cleanCodeAttribute and addImpact of NewExternal Issue that could be removed in a near future by @alban-auzeill in Remove usage of @Beta cleanCodeAttribute and addImpact of NewExternal Issue that could be removed in a near future sonar-analyzer-commons#306

sonar-csharp-plugin/pom.xml

antonioaversa

Mostly remarks on the use of variables, questions on the dependency management across pom files and improvements on logging.

antonioaversa · 2023-08-14T08:29:23Z

pom.xml

@@ -57,25 +57,39 @@
    <artifactsToDownload>${project.groupId}:SonarAnalyzer.CSharp:nupkg,${project.groupId}:SonarAnalyzer.VisualBasic:nupkg</artifactsToDownload>
    <!-- We are ignoring java doc warnings - this is because we are using JDK 11. Ideally we should not do that. -->


Educational: is this comment still relevant? Are we still using JDK 11?
Should we rather remove <doclint>none</doclint>, since we are moving to JDK 17?
Or have a future task to do so?

I'm not sure about this but it's not related to this PR. Could you please create a task to check and remove if possible?

pom.xml

sonar-csharp-plugin/pom.xml

antonioaversa · 2023-08-14T12:12:30Z

sonar-dotnet-shared-library/pom.xml

@@ -36,6 +36,11 @@
      <version>${findbugs.version}</version>


Educational: both com.google.code.findbugs::jsr305 and org.slf4j::slf4j-api are defined as provided dependency, yet one relies on the version defined at org.sonarsource.dotnet::sonar-dotnet level, in the dependencyManagement, whereas the other relies on explicit versioning and properties inheritance.
Any reason for the difference in approach? Could we make the approach uniform, or explain the difference?

I've missed this one. Thanks!

antonioaversa · 2023-08-14T12:15:25Z

...-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/CoverageReportImportSensor.java

@@ -120,7 +121,9 @@ void analyze(SensorContext context, Coverage coverage) {
    LOG.debug("The total number of file count statistics is '{}'.", fileCountStatistics.total);

    if (fileCountStatistics.total != 0) {
-      LOG.info(fileCountStatistics.toString());
+      if (LOG.isInfoEnabled()) {


As discussed offline, I would avoid adding those if (LOG.isXEnabled) before logging at Y level, because it may introduce issues (e.g. when X is different than Y) and also makes the code less readable.

Instead, I would adopt the approach defined here. The day the facade will support Log4j lazy logging API, we will either change the signature of the lazy method, to return the input Callable<?>, or we would just drop the lazy call, unwrapping its parameter.

antonioaversa · 2023-08-14T12:45:34Z

...t-shared-library/src/main/java/org/sonar/plugins/dotnet/tests/DotCoverReportsAggregator.java

@@ -44,7 +44,9 @@ public class DotCoverReportsAggregator implements CoverageParser {

  @Override
  public void accept(File file, Coverage coverage) {
-    LOG.debug("The current user dir is '{}'.", System.getProperty("user.dir"));
+    if (LOG.isDebugEnabled()) {


As above.

Applies to all occurrences of if (LOG.is in the changes below.

antonioaversa · 2023-08-14T12:54:04Z

...otnet-shared-library/src/main/java/org/sonarsource/dotnet/shared/plugins/FileTypeSensor.java

-      LOG.debug("Adding file type information (has MAIN '{}', has TEST '{}') for project '{}' (project key '{}', base dir '{}'). For debug info, see ProjectInfo.xml in '{}'.",
-        hasMainFiles, hasTestFiles, getValueOrEmpty(configuration, PROJECT_NAME_PROPERTY),
-        getValueOrEmpty(configuration, PROJECT_KEY_PROPERTY), getValueOrEmpty(configuration, PROJECT_BASE_DIR_PROPERTY), analyzerWorkDir.get());
+      if (LOG.isDebugEnabled()) {


Maybe for this case specifically, it makes sense to keep the if (LOG.isXEnabled()), because the alternative is wrapping each of the 6 parameters into a lazy.

I've changed it anyway to be consistent.

sonar-vbnet-plugin/pom.xml

antonioaversa

LGTM, thanks for all the explanations!
I left a minor comment about inlining a short ternary operator.
It is optional, and in case can come in a later PR.

Just a remark: we have introduced quite a few deprecation-related Code Smell, so SC Quality Gate is failing on Maintainability.
We will catch up soon on that, removing calls to set severity and rule type, in favor of quality, impact and CCT attribute.

antonioaversa · 2023-08-15T08:33:52Z

sonar-dotnet-shared-library/src/main/java/org/sonarsource/dotnet/shared/CallableUtils.java

+      public String toString() {
+        try {
+          Object result = callable.call();
+          return result == null


I would write this ternary inline, since it's short and simple to understand.

sonarqubecloud · 2023-08-15T09:32:54Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
30 Code Smells

98.9% Coverage
0.3% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

sonarqubecloud · 2023-08-15T09:38:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

github-actions bot assigned costin-zaharia-sonarsource Aug 10, 2023

costin-zaharia-sonarsource force-pushed the costin/update-plugin-api branch 8 times, most recently from 6e8f05d to c90487c Compare August 14, 2023 07:05

costin-zaharia-sonarsource mentioned this pull request Aug 14, 2023

Bump sonar.analyzer.commons.version from 2.5.0.1358 to 2.7.0.1482 #7787

Closed

costin-zaharia-sonarsource marked this pull request as ready for review August 14, 2023 08:16

costin-zaharia-sonarsource requested a review from antonioaversa August 14, 2023 08:17

github-actions bot assigned antonioaversa and unassigned costin-zaharia-sonarsource Aug 14, 2023

costin-zaharia-sonarsource commented Aug 14, 2023

View reviewed changes

sonar-csharp-plugin/pom.xml Show resolved Hide resolved

antonioaversa requested changes Aug 14, 2023

View reviewed changes

github-actions bot assigned costin-zaharia-sonarsource and unassigned antonioaversa Aug 14, 2023

costin-zaharia-sonarsource force-pushed the costin/update-plugin-api branch 6 times, most recently from 3aaf945 to 4d3825e Compare August 15, 2023 07:13

costin-zaharia-sonarsource requested a review from antonioaversa August 15, 2023 07:15

github-actions bot assigned antonioaversa and unassigned costin-zaharia-sonarsource Aug 15, 2023

antonioaversa approved these changes Aug 15, 2023

View reviewed changes

github-actions bot assigned costin-zaharia-sonarsource and unassigned antonioaversa Aug 15, 2023

costin-zaharia-sonarsource added 10 commits August 15, 2023 11:11

Bump sonar-plugin-api to 10.1.0.809

65ac4db

Update min sonarqube version

52c0214

Replace Loggers.get with LoggerFactory.getLogger

5e82891

Replace string concatenation

f827529

Replace org.sonar.api.utils.log.LoggerLevel; with org.slf4j.event.Level;

84c8214

Check if logging is enabled before doing invocations

23182dd

Bump sonar-analyzer-commons to 2.7.0.1482

f97a7db

Use lazy instead of conditional logging

750daf8

Address comments

8822ad7

Inline ternary expression

1dfc905

costin-zaharia-sonarsource force-pushed the costin/update-plugin-api branch from 4d3825e to 1dfc905 Compare August 15, 2023 09:13

costin-zaharia-sonarsource merged commit 596c8e9 into master Aug 15, 2023

costin-zaharia-sonarsource deleted the costin/update-plugin-api branch August 15, 2023 10:01

This was referenced Aug 18, 2023

Pin sonar-dotnet to 9.7 in old SQ ITs SonarSource/sonar-scanner-msbuild#1663

Merged

Pin sonar-dotnet version in unsupported LTS SonarQube integration tests SonarSource/sonar-scanner-msbuild#1664

Closed

This was referenced Jul 23, 2024

Java UTs: Replace LogTester with slf4j #7600

Closed

Replace org.sonar.api.utils.log.Logger #7115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the plugin api #7777

Update the plugin api #7777

costin-zaharia-sonarsource commented Aug 10, 2023 •

edited by antonioaversa

Loading

antonioaversa left a comment

antonioaversa Aug 14, 2023

costin-zaharia-sonarsource Aug 14, 2023

antonioaversa Aug 14, 2023

costin-zaharia-sonarsource Aug 14, 2023

antonioaversa Aug 14, 2023

antonioaversa Aug 14, 2023

antonioaversa Aug 14, 2023

costin-zaharia-sonarsource Aug 14, 2023

antonioaversa left a comment •

edited

Loading

antonioaversa Aug 15, 2023

sonarqubecloud bot commented Aug 15, 2023

sonarqubecloud bot commented Aug 15, 2023

		@@ -57,25 +57,39 @@
		<artifactsToDownload>${project.groupId}:SonarAnalyzer.CSharp:nupkg,${project.groupId}:SonarAnalyzer.VisualBasic:nupkg</artifactsToDownload>
		<!-- We are ignoring java doc warnings - this is because we are using JDK 11. Ideally we should not do that. -->

Update the plugin api #7777

Update the plugin api #7777

Conversation

costin-zaharia-sonarsource commented Aug 10, 2023 • edited by antonioaversa Loading

Update of sonar-analyzer-commons

antonioaversa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonioaversa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 15, 2023

sonarqubecloud bot commented Aug 15, 2023

costin-zaharia-sonarsource commented Aug 10, 2023 •

edited by antonioaversa

Loading

antonioaversa left a comment •

edited

Loading