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

refactor: Add facade around AnalyzerMessage #163

Merged

Conversation

slarse
Copy link
Collaborator

@slarse slarse commented Oct 21, 2020

This is a follow-up on #162, and depends on it. Don't review this until #162 i smerged, I will fix the commit history.

This PR extracts the previously internal SoraldAbstractProcessor.Bug class into a separate sorald.sonar.RuleViolation class, which is just a thin facade around org.java.sonar.AnalyzerMessage. Depending on how we proceed with #156, we may not have access to the actual AnalyzerMessage (but some higher-level abstraction instead), and so this PR is a precaution.

Since the facade was essentially already there in form of the Bug class, this change was very little effort, and also coincidentally cleans up some of the code base. With this change, the only sonar internals we touch in sorald are the check classes, but it doesn't seem reasonable at the moment to put adapters around them. I think we'll leave them be.

@slarse slarse added the WiP label Oct 21, 2020
@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #163 into master will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #163      +/-   ##
============================================
- Coverage     83.44%   83.36%   -0.08%     
- Complexity      354      355       +1     
============================================
  Files            37       38       +1     
  Lines          1558     1551       -7     
  Branches        214      212       -2     
============================================
- Hits           1300     1293       -7     
  Misses          158      158              
  Partials        100      100              
Impacted Files Coverage Δ Complexity Δ
src/main/java/sorald/miner/MineSonarWarnings.java 74.01% <100.00%> (+0.41%) 13.00 <0.00> (ø)
...java/sorald/processor/SoraldAbstractProcessor.java 85.48% <100.00%> (-2.98%) 23.00 <0.00> (-2.00)
src/main/java/sorald/sonar/RuleVerifier.java 90.00% <100.00%> (+2.50%) 4.00 <1.00> (ø)
src/main/java/sorald/sonar/RuleViolation.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 853e9a0...82fda82. Read the comment docs.

@slarse slarse force-pushed the issue/89-extract-bug-as-message-facade branch from 124faf1 to 82fda82 Compare October 21, 2020 13:20
@slarse slarse requested a review from fermadeiral October 21, 2020 13:25
@slarse
Copy link
Collaborator Author

slarse commented Oct 21, 2020

@fermadeiral Ready for review! Code coverage drop is due to removal of code.

@fermadeiral
Copy link
Collaborator

@slarse, thank you for this refactoring.

I noticed you have added documentation in the source code since previous PRs. That is great!

@fermadeiral fermadeiral merged commit e11f354 into ASSERT-KTH:master Oct 21, 2020
@slarse
Copy link
Collaborator Author

slarse commented Oct 21, 2020

@fermadeiral Glad you like it! I tend to add documentation to most of my code. I enjoy being tidy :)

Sometimes I add too much documentation as I like writing, so you may want to be on the lookout for overly verbose docs where something less verbose would do just fine.

@slarse slarse deleted the issue/89-extract-bug-as-message-facade branch October 21, 2020 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants