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] Adding the fix for #1440. Showing correct message for CommentDefaultAccessmodifier. #1453

Merged
merged 2 commits into from Nov 21, 2018

Conversation

Projects
None yet
5 participants
@stationeros
Copy link

stationeros commented Nov 11, 2018

Fixes #1440. Changed the grammar to set the image for the node, thereby resulting in the name of the constructor to be appearing in the error message.

Rohit Kumar Rohit Kumar
@pmd-test

This comment has been minimized.

Copy link

pmd-test commented Nov 11, 2018

1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 11, 2018

@stationeros thank for the PR! This looks great!

Just a couple of minor comments:

  1. Consider defining a different variable for the token, as t is already being used.
  2. We should add a test-case to make sure the fix is actually working. You should add a test for the CommentDefaultAccessModifier rule checking the error message matches expectations. You can see how that is done for instance here:

<expected-problems>5</expected-problems>
<expected-messages>
<message>The method parameter name 'Foo' doesn't match '[a-z][a-zA-Z0-9]*'</message>

@jsotuyod jsotuyod added the is:WIP label Nov 12, 2018

@jsotuyod jsotuyod added this to the 6.10.0 milestone Nov 12, 2018

@adangel adangel removed the is:WIP label Nov 21, 2018

@adangel
Copy link
Member

adangel left a comment

Awesome, thanks for the PR!

@adangel adangel merged commit 874b528 into pmd:master Nov 21, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

adangel added a commit that referenced this pull request Nov 21, 2018

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