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] New Rule - UseUnderscoresInNumericLiterals #1384

Merged
merged 13 commits into from Nov 10, 2018

Conversation

Projects
None yet
5 participants
@rajeshggwp
Copy link
Collaborator

rajeshggwp commented Oct 14, 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:
Resolves #1232

@jsotuyod

This comment has been minimized.

this is only supported in Java 7+, so you should add a minimumLanguageVersion="1.7" here to make sure we don't report this on older versions

@jsotuyod

This comment has been minimized.

the correct version would be 6.9.0

@jsotuyod

This comment has been minimized.

this can be slightly more concise by writing it as: "^[0-9]{1,3}(_[0-9]{3})*(l|L|\.[0-9]+(d|D|f|F)?)?$")

@jsotuyod

This comment has been minimized.

I'd add a couple more test scnearios:

  1. Actually using l / L / d / D / F (you do have an f scenario)
  2. A scenario where underscores are used but in the wrong positions, ie: 23_05 or 23_3456_333

@rajeshggwp rajeshggwp changed the title [java] #1232 New Rule - NumericLiteralConvention [java] pmd#1232 New Rule - NumericLiteralConvention Oct 14, 2018

@rajeshggwp rajeshggwp changed the title [java] pmd#1232 New Rule - NumericLiteralConvention [java] New Rule - NumericLiteralConvention Oct 14, 2018

@pmd-test

This comment has been minimized.

Copy link

pmd-test commented Oct 14, 2018

1 Message
📖 This changeset introduces 550 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 596 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 585 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 1819 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 1819 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 1819 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 2330 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report
This changeset introduces 193282 new violations and 0 new errors,
removes 0 violations and 2 errors. Full report

Generated by 🚫 Danger

@jsotuyod jsotuyod self-assigned this Oct 14, 2018

@jsotuyod jsotuyod added the a:new-rule label Oct 14, 2018

@jsotuyod jsotuyod added this to the 6.9.0 milestone Oct 14, 2018

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Oct 14, 2018

@rajeshggwp thanks for the PR!

The rule seems to be introducing false positives, for instance, it's reporting on this line:

https://github.com/checkstyle/checkstyle/blob/9cf091d553be0567e68e0439f56f12c63c496e18/src/it/java/com/google/checkstyle/test/base/AbstractIndentationTestSupport.java#L41

I'd add a new test case reproducing the issue, and then work on fixing it. Pushing to your branch will have the regression tester rerun and update results here automatically :)

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Oct 15, 2018

The rule is working much better now! Looking at the pmd-test results I see however some scenarios we could handle better:

  1. serialVersionUID is autogenerated and developers don't really care about reading it. We should probably not report on that field
  2. numbers that are not in base 10 shouldn't be reported (if not using base 10, the 3 digit rule is pointless). You can discard literals starting with '0' (covers all octal, hexa and binary), ie: 0x1234, 0723, 0b101010. Be careful with those starting with 0. 'though...
  3. your regexp is not considering scientific notation. -2.147483648E10 is valid.
  4. your regexp is not allowing underscores in the decimal part. I can understand you not wanting to enforce them, but I'm unsure disallowing them altogether is sane. In the previous example, I'd personally think -2.147_483_648E10 is clearer to read.

Finally, even though your rule handles it properly, I'd like to include a couple tests with negative numbers (even negatives in bases other than 10, as per the second item). It should just work as the rule stands, but keeping it safe with a test. ie: -1_000, -0x1234, -0723, -0b101010

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

@oowekyala
Copy link
Member

oowekyala left a comment

@rajeshggwp Thanks for taking this on!

@adangel adangel modified the milestones: 6.9.0, 6.10.0 Oct 27, 2018

@rajeshggwp rajeshggwp force-pushed the rajeshggwp:master branch from a48edb6 to 4fadb1a Nov 7, 2018

rajeshggwp added some commits Nov 8, 2018

@oowekyala
Copy link
Member

oowekyala left a comment

This seems nearly ready. I'd like to see a test case for when the underscores are misplaced (eg -11_476_8_9), when it's under the acceptable length, and this should be good to go

@rajeshggwp rajeshggwp changed the title [java] New Rule - NumericLiteralConvention [java] New Rule - UseUnderscoresInNumericLiterals Nov 8, 2018

@jsotuyod jsotuyod removed the is:WIP label Nov 9, 2018

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 9, 2018

This is awesome! It seems ready to merge. I'll do so when I get the chance.

@oowekyala
Copy link
Member

oowekyala left a comment

LGTM, many thanks for your work @rajeshggwp!

@jsotuyod jsotuyod merged commit 750381c into pmd:master Nov 10, 2018

1 check passed

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

jsotuyod added a commit that referenced this pull request Nov 10, 2018

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