[WIP] resolve SonarCloud violations #221
Conversation
muhlba91
changed the title
| private static void loadHtmlDescriptions(NewRepository repository, String htmlDescriptionFolder) { | ||
| // code adapted from: | ||
| // https://github.com/SonarSource/sslr-squid-bridge/blob/2.7.0.377/ | ||
| // src/main/java/org/sonar/squidbridge/rules/ExternalDescriptionLoader.java |
There was a problem hiding this comment.
Can we move these comments to the Javadoc?
Maybe copy the first sentence from sonar's javadoc (if there is one) and append a note saying it was copied from and use {@link ExternalDescriptionLoader#loadHtmlDescriptions(NewRepository, String)} instead of url to github.
There was a problem hiding this comment.
Yes, we can but shouldn't we also move https://github.com/checkstyle/sonar-checkstyle/blob/master/src/main/java/org/sonar/plugins/checkstyle/CheckstyleRulesDefinition.java#L73 to the Javadoc as it follows the same?
However, I don't quite understand what you mean with {@link ExternalDescriptionLoader#loadHtmlDescriptions(NewRepository, String)}?
Because for now, this class still exists, just like loadNames (see link before) still existed until they removed some of their deprecated code. I assume this will happen here as well, meaning ExternalDescriptionLoader won't exist at some point anymore.
Both methods don't have any Javadoc in sslr-squid-bridge.
There was a problem hiding this comment.
shouldn't we also move ... to the Javadoc as it follows the same?
Yes I like that.
Because for now, this class still exists, just like loadNames (see link before) still existed until they removed some of their deprecated code. I assume this will happen here as well, meaning ExternalDescriptionLoader won't exist at some point anymore.
Ok, URL has tag in it so it will never go bad unless github goes away.
{@link was just a javadoc tag that would take user to that method if they clicked on it like in an IDE. If class/method does go away, javadoc tool should print an error on the {@link as it won't point to a valid class.
I was mainly just pointing its usage since I feel it is a better way to point to a class/method in a javadoc. If everyone wants to stay with URL, I am fine.
There was a problem hiding this comment.
For this point I was waiting for a decision if we want to put dead Javadoc links in or stick with GitHub URL references?
I'm good with both ways but we should still keep the URL somewhere as well for a full reference to where the code was taken from (incl. sslr-squid-bridge version). Otherwise, you won't really know from where/when the code was taken over and whether it was modified by us already or not.
There was a problem hiding this comment.
I see no reason in reference of javadoc in other project, especially to non-existing code (non-existing javadoc).
Please move link from single comment to loadHtmlDescriptions javadoc styles comment. Please do the same in "loadNames". Simply move whole text of comment to javadoc (no extra descriptions are required.)
|
Please do not try to fix all problems in one commit and one PR. Please split the work to ease review |
|
Code coverage is failing on travis. |
|
@romani sorry, these weeks, unfortunately, I'm quite busy. This is why development of this issue has been a bit stalled. I will think about how to split it into more commints and PR, so either per violation or per rule. I'm not entirely sure which one is more useful though? Then I'm going to close this PR but will go to #222 first. |
it is up to you, it is my recommendation. Long/Big PRs are prone to stay in long review and usually one questionable code piece keep whole PR. So it is recommendation to do all step by step or in smaller pieces that easy to consume by reviewer and in case of not-simple cases other fixes are not blocked. |
|
@muhlba91 , ping. Please move comment to javadoc and lets merge this PR. |
|
@muhlba91 , please do final update and lets merge this PR |
|
@muhlba91 , can you finish this PR ? or should we close it ? FYI: I updated checkstyle project to actually use Sonar as CI validator for PRs, it would be awesome to eventually Sonar to control our code and in this plugin. |
|
i'll close it for now and work on it later. |
fixes #217;
except violations relating to #219 and #220.