-
Notifications
You must be signed in to change notification settings - Fork 181
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
Conversation
...script-plugin/src/main/java/org/sonar/plugins/javascript/analysis/cache/CacheStrategies.java
Show resolved
Hide resolved
...plugin/bridge/src/main/java/org/sonar/plugins/javascript/bridge/JavaScriptFilePredicate.java
Outdated
Show resolved
Hide resolved
sonar-plugin/bridge/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServerImpl.java
Show resolved
Hide resolved
sonar-plugin/bridge/src/main/java/org/sonar/plugins/javascript/bridge/EslintRule.java
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation
sonar-plugin/bridge/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java
Show resolved
Hide resolved
sonar-plugin/bridge/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java
Show resolved
Hide resolved
sonar-plugin/bridge/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java
Show resolved
Hide resolved
sonar-plugin/bridge/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java
Show resolved
Hide resolved
...in/bridge/src/test/java/org/sonar/plugins/javascript/bridge/JavaScriptFilePredicateTest.java
Outdated
Show resolved
Hide resolved
...vascript-plugin/src/main/java/org/sonar/plugins/javascript/analysis/cache/CpdSerializer.java
Show resolved
Hide resolved
.../sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServer.java
Outdated
Show resolved
Hide resolved
...ascript-plugin/src/test/java/org/sonar/plugins/javascript/analysis/cache/CacheTestUtils.java
Show resolved
Hide resolved
There was a problem hiding this 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.
There was a problem hiding this 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!
|
Some changes were made in
BridgeServer
to improve the overall coderecord
which makes the fieldsfinal
and private, they are available through accessor methods (thus change of field access to method invocation()
)js,ts,yaml
) to avoid circular dependency betweenbridge
andsonar-javascript-plugin
public
, because there is too much sharing betweenbridge
andsonar-javascript-plugin
, when we will migrate more logic to the Node.js part, we will be able to improve this