-
Notifications
You must be signed in to change notification settings - Fork 70
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
Issue #317: New metadata maintanence method: Fetch directly from checketyle #318
Conversation
c58b53a
to
b2ddf41
Compare
please post a picture of the check being triggered - an analysis with code that fails the new checks. |
But this should be done in checkstyle upgrade PR like #308 right? This is just to show that |
b2ddf41
to
29850fe
Compare
@gaurabdg , CI is red, so there is no prove that it is working. lets use: make CI green. |
I had same problem at checkstyle/checkstyle#8768 (comment) , I fixed it in checkstyle, but looks like there is one more problem. from CI
|
I reproduced problem on my local:
|
but it works fine with version |
full stracktrace:
caused by |
e1034b5
to
7f09ab8
Compare
Manual Tests:
Shown in PR description
new "test" profile created to test default config:
Activating EmptyBlock with
|
@gaurabdg , please fix CI - https://travis-ci.org/github/checkstyle/sonar-checkstyle/jobs/722154861#L319
|
7f09ab8
to
318d297
Compare
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.
items:
src/test/java/org/sonar/plugins/checkstyle/internal/ChecksTest.java
Outdated
Show resolved
Hide resolved
318d297
to
08c14b1
Compare
08c14b1
to
055b277
Compare
… from checkstyle
055b277
to
a44068f
Compare
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.
item:
@@ -118,6 +269,10 @@ private static void validateSonarRules(Document document, Set<Class<?>> modules) | |||
|
|||
final String key = rule.getAttributes().getNamedItem("key").getTextContent(); | |||
|
|||
if (!PRE_CHECKSTYLE_8_36_MODULES.contains(key)) { |
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.
please filter out all modules that from version 8.36 and above from final Set<Class<?>> modules = CheckUtil.getCheckstyleModules();
so you will need to filter out ones only.
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.
moved to #323
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.
No, it wouldnt help.
Assert.assertNotNull("Unknown class found in sonar rules"
+ " (" + RULES_PATH + ")" + " :" + key, module);
This would get executed for checks which would be filtered out, but rules.xml
will still contain it, so we will end up having additional checks.
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.
@muhlba91, please take a look
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.
@gaurabdg @romani i only see a "clean code" benefit here but not a performance related one as we are using a HashSet
, which has lookups of O(1) in this case.
@gaurabdg what i'd rather see is a short comment wherever this filter is used why exactly we need it at this place.
i believe #322 and #321 are of high importance and should be fixed now. next up probably #324 and this one can be looked at last.
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.
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.
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
One bug that could be done after this PR is merged - #322 |
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.
update is small
as update to 8.36 must be done, I am merging this PR we can improve further in other PR/issues
#317
RecordComponentNumberCheck
(to be released in8.36
) metadata is successfully loaded from checkstyle, without manual entry inrules.xml
178
from174
.