Skip to content

Conversation

@henryju
Copy link
Member

@henryju henryju commented Jan 8, 2020

No description provided.

@henryju henryju force-pushed the feature/java_support branch from 0030b69 to d438041 Compare February 24, 2020 14:50
@henryju henryju changed the title [WIP] Add support for Java analysis Add support for Java analysis Feb 24, 2020
@henryju henryju force-pushed the feature/java_support branch from 7194900 to 454d1ad Compare February 25, 2020 09:28
@henryju henryju requested a review from jblievremont February 25, 2020 09:57
@henryju henryju self-assigned this Feb 25, 2020
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 comments 😄


Optional<ProjectBindingWrapper> binding = bindingManager.getBinding(fileUri);
AnalysisResultsWrapper analysisResults;
final Optional<GetJavaConfigResponse> javaConfigOptFinal = javaConfigOpt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make it final from the beginning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, that's a pre-refactoring remain

}
}

private void analyzeAllOpenJavaFiles() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether it would make sense to refactor these analyzeAllXxx methods, e.g with a method named analyzeFiles(Predicate<URI>).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think using predicates will make it harder to understand what this is doing. Just to save the loop, I don't think it worth factorizing. WDYT?

analyzeAllOpenJavaFiles();
}

private boolean isTest(WorkspaceFolderSettings settings, URI fileUri, Optional<GetJavaConfigResponse> javaConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@JsonRequest("sonarlint/getJavaConfig")
CompletableFuture<GetJavaConfigResponse> getJavaConfig(String fileUri);

public static class GetJavaConfigResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: might be worth a Javadoc link to the reference

@henryju henryju force-pushed the feature/java_support branch from 2e6774a to 67e55c9 Compare February 27, 2020 14:17
@henryju henryju merged commit 3102096 into master Feb 27, 2020
@henryju henryju deleted the feature/java_support branch February 27, 2020 14:18
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