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

[java] Fix Issue 1343: Update CommentDefaultAccessModifierRule #1400

Merged
merged 3 commits into from Oct 27, 2018

Conversation

Projects
None yet
4 participants
@CrazyUnderdog
Contributor

CrazyUnderdog commented Oct 19, 2018

Before submitting a PR, please check that:

  • The PR is submitted against master. The PMD team will merge back to support branches as needed.
  • ./mvnw clean verify passes. This will build and test PMD, execute PMD and checkstyle rules. Check this for more info

PR Description:

Fixes #1343, the class CommentDefaultAccessModifierRule now extends AbstractIgnoredAnnotationRule. Also added a small comment about the ignored nodes in CommentDefaultAccessModifierRule.

Jan Brümmer
@pmd-test

This comment has been minimized.

pmd-test commented Oct 19, 2018

1 Message
📖 This changeset introduces 0 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report

Generated by 🚫 Danger

@adangel

Thanks for the PR!

Extending AbstractIgnoredAnnotationRule is however only part of the work needed here:

@adangel adangel changed the title from [java] Fix Issue-1343: to [java] Fix Issue 1343: Update CommentDefaultAccessModifierRule Oct 19, 2018

@adangel adangel added this to the 6.9.0 milestone Oct 19, 2018

@CrazyUnderdog CrazyUnderdog reopened this Oct 19, 2018

@Override
protected Collection<String> defaultSuppressionAnnotations() {
Collection<String> ignoredStrings = new ArrayList<>();
ignoredStrings.add("VisibleForTesting");

This comment has been minimized.

@jsotuyod

jsotuyod Oct 19, 2018

Member

you should use fully qualified class names here for proper type matching.

the ones we probably care about are:
com.google.common.annotations.VisibleForTesting
android.support.annotation.VisibleForTesting

@@ -41,6 +41,14 @@
public CommentDefaultAccessModifierRule() {
definePropertyDescriptor(REGEX_DESCRIPTOR);
defaultSuppressionAnnotations();

This comment has been minimized.

@jsotuyod

jsotuyod Oct 19, 2018

Member

there is no need to call this method here

This comment has been minimized.

@adangel

adangel Oct 19, 2018

Member

Yes, this is called already by the super class, when it is defining the property:

@jsotuyod jsotuyod added the is:WIP label Oct 19, 2018

@CrazyUnderdog

This comment has been minimized.

Contributor

CrazyUnderdog commented Oct 24, 2018

I can take a look at this at the weekend. However I want you to know that I did this pull request as a part of an event, and can't put much time into this anymore.

@CrazyUnderdog

This comment has been minimized.

Contributor

CrazyUnderdog commented Oct 27, 2018

Okay, for whatever reason, my IDE does not want to import your codestyle, so that check still fails.

I've removed the unnecessary method all and added the full class paths to the ignoredStrings collection.

@adangel adangel self-assigned this Oct 27, 2018

@adangel

This comment has been minimized.

Member

adangel commented Oct 27, 2018

@CrazyUnderdog Thanks for your time! I'll take it from here

@adangel adangel removed the is:WIP label Oct 27, 2018

@adangel adangel merged commit ae97d9f into pmd:master Oct 27, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

adangel added a commit that referenced this pull request Oct 27, 2018

@adangel

This comment has been minimized.

Member

adangel commented Oct 27, 2018

@CrazyUnderdog Thanks for the PR, merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment