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.

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

JS-161 Separate bridge from sonar-javascript-plugin #4697

Merged
merged 21 commits into from
May 24, 2024
Merged

JS-161 Separate bridge from sonar-javascript-plugin #4697

merged 21 commits into from
May 24, 2024

Conversation

saberduck
Copy link
Contributor

@saberduck saberduck commented May 20, 2024

Some changes were made in BridgeServer to improve the overall code

  • use of Java record which makes the fields final and private, they are available through accessor methods (thus change of field access to method invocation () )
  • removed some deprecated code related to logging, mostly in tests
  • use of String literals for language keys (js,ts,yaml) to avoid circular dependency between bridge and sonar-javascript-plugin
  • some methods are forced to be public, because there is too much sharing between bridge and sonar-javascript-plugin, when we will migrate more logic to the Node.js part, we will be able to improve this

@saberduck saberduck changed the title Separate bridge from sonar-javascript-plugin JS-161 Separate bridge from sonar-javascript-plugin May 23, 2024
pom.xml Show resolved Hide resolved
@@ -185,7 +183,7 @@ void should_get_answer_from_server() throws Exception {
.setContents("alert('Fly, you fools!')")
.build();
JsAnalysisRequest request = createRequest(inputFile);
assertThat(bridgeServer.analyzeJavaScript(request).issues).isEmpty();
assertThat(bridgeServer.analyzeJavaScript(request).issues()).hasSize(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

how come we are getting an issue now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the startServer script to return some issue, in order to have code coverage in BridgeServer

see here https://github.com/SonarSource/SonarJS/pull/4697/files#diff-d348a0b26e6add7d9a6e42a713de34abcd9358b9fe5db19ddb816145573ffe8dR46

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation

Copy link
Contributor

@ilia-kebets-sonarsource ilia-kebets-sonarsource left a comment

Choose a reason for hiding this comment

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

a few questions about:

  • test results changing
  • language constants being a bit ugly
  • minor questions and remarks

Otherwise LGTM. Let me know if you wish togo through it together.

Copy link
Contributor

@ilia-kebets-sonarsource ilia-kebets-sonarsource left a comment

Choose a reason for hiding this comment

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

Thank you for the clarifications, LGTM!

@saberduck saberduck merged commit 6789d31 into master May 24, 2024
14 of 15 checks passed
@saberduck saberduck deleted the JS-161 branch May 24, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants