Skip to content

Java analysis support #68

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.

Sign up for GitHub

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

Jump to bottom
Merged
henryju merged 7 commits into from
Feb 28, 2020
Merged

Java analysis support #68

henryju merged 7 commits into from
Feb 28, 2020

Conversation

@henryju
Copy link
Member

@henryju henryju commented Jan 8, 2020 •
edited
Loading

See discussion on #28

Related changes on LS side:
SonarSource/sonarlint-language-server#10

@henryju henryju changed the title Java support [WIP] Java support Jan 8, 2020
@henryju henryju mentioned this pull request Jan 13, 2020
Closed
@henryju henryju force-pushed the feature/java_support branch from 539defd to dd73723 Compare January 14, 2020 15:01
@henryju henryju force-pushed the feature/java_support branch from dd73723 to 35b2c2e Compare February 25, 2020 14:07
@henryju henryju force-pushed the feature/java_support branch from 35b2c2e to ee9b6c7 Compare February 25, 2020 14:08
@henryju henryju changed the title [WIP] Java support Java analysis support Feb 25, 2020
@henryju henryju requested a review from jblievremont February 25, 2020 14:15
@henryju henryju self-assigned this Feb 25, 2020
@henryju henryju force-pushed the feature/java_support branch from 93c5848 to a68ce82 Compare February 25, 2020 15:39
@henryju henryju force-pushed the feature/java_support branch 4 times, most recently from abd16ec to 20b9bb1 Compare February 25, 2020 18:25
@henryju henryju force-pushed the feature/java_support branch from 20b9bb1 to 9d38d81 Compare February 25, 2020 18:26
@jdneo
Copy link

jdneo commented Feb 26, 2020

Hi @henryju, the PR looks good to me. :shipit:

jblievremont
jblievremont approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@jblievremont jblievremont left a comment

Choose a reason for hiding this comment

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

LGTM with a few minor comments.

Tested:
✔️ Offline single module Maven project
✔️ Offline rule activation/deactivation
✔️ Single module Maven project in connected mode
✔️ Offline multi module Maven project
✔️ Multi module Maven project in connected mode

CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## 1.15.0

* Add support for Java analysis
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

I think we should mention the soft dependency on vscode-java v0.56+, either here or in the Requirements section in README.md

its/samples/sample-java-maven-multi-module/.gitignore Outdated
Comment on lines 1 to 8
# The following should be moved in related sub-directories
server/sonar-web/src/main/webapp/stylesheets/sonar-colorizer.css
server/sonar-web/src/main/webapp/deploy/plugins
server/sonar-web/src/main/webapp/deploy/bootstrap
server/sonar-web/src/main/webapp/deploy/maven/org
server/sonar-web/src/main/webapp/WEB-INF/log/
server/sonar-web/src/main/webapp/deploy/*.jar
server/sonar-web/src/main/webapp/deploy/jdbc-driver.txt
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

Nitpick: this .gitignore could be leaner, e.g with only stuff related to Maven, IDE and OS

its/samples/workspace-java.code-workspace Outdated
{
"folders": [
{
"path": "/home/julien/Prog/Projects/sonarlint/sonarlint-vscode/its/samples/sample-java-maven-multi-module"
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

Not sure of this one 😅

Copy link
Member Author

@henryju henryju Feb 26, 2020

Choose a reason for hiding this comment

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

Good catch. I wonder how it is possible that ITs are passing on Azure with this mistake. Maybe VSCode is smart enough to fallback on local folders...

package-lock.json
"which": {
"version": "1.0.9",
"resolved": "https://repox.jfrog.io/repox/api/npm/npm/which/-/which-1.0.9.tgz",
"resolved": "https://registry.npmjs.org/which/-/which-1.0.9.tgz",
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

Nitpick: this diff should not happen (maybe repox was unavailable when you ran npm install?)

package-lock.json
"winreg": {
"version": "1.2.4",
"resolved": "https://repox.jfrog.io/repox/api/npm/npm/winreg/-/winreg-1.2.4.tgz",
"resolved": "https://registry.npmjs.org/winreg/-/winreg-1.2.4.tgz",
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

Same as above

src/extension.ts Outdated

languageClient.onReady().then(() =>
languageClient.onReady().then(() => {
languageClient.onRequest(ShowRuleDescriptionRequest.type, params => {
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

For consistency with the other request declaration, this ought to be extracted to a named function.

src/extension.ts Outdated
installClasspathListener();
}

function installClasspathListener() {
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

Nitpick: we could extract functions related to support of Java in a dedicated module (e.g java.ts), to avoid clutter in extension.ts

@henryju henryju force-pushed the feature/java_support branch from 6ea0ad2 to 2ebf6bb Compare February 26, 2020 12:22
jblievremont
jblievremont reviewed Feb 26, 2020
View reviewed changes
src/extension.ts
let languageClient: SonarLintExtendedLanguageClient;

function logToSonarLintOutput(message) {
export function logToSonarLintOutput(message) {
Copy link
Contributor

@jblievremont jblievremont Feb 26, 2020

Choose a reason for hiding this comment

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

I wonder whether we should extract logging stuff to a dedicated module also. As it stands now, there is a cyclic dependency between extension.ts and java.ts - not a big issue, but annoying nonetheless.

@henryju henryju force-pushed the feature/java_support branch from 2d17c6b to 6a19342 Compare February 27, 2020 08:45
@henryju henryju merged commit 4e0599f into master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jblievremont jblievremont jblievremont approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@henryju henryju

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@henryju @jdneo @jblievremont