Skip to content

[java] New rule: UseStandardCharsets#3193

Merged
adangel merged 2 commits into
pmd:masterfrom
aaime:useStandardCharsets
Apr 10, 2021
Merged

[java] New rule: UseStandardCharsets#3193
adangel merged 2 commits into
pmd:masterfrom
aaime:useStandardCharsets

Conversation

@aaime

@aaime aaime commented Apr 3, 2021

Copy link
Copy Markdown
Contributor

Describe the PR

(My first PR against PMD. Hopefully I did not screw up too badly :-D)

Starting with Java 7, StandardCharsets provides constants for common Charset objects, such as UTF-8.
Using the constants is less error prone, and can provide a small performance advantage compared to Charset.forName(...)
since no scan across the internal Charset caches is needed.

More discussion available in the linked issue

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

Unsure about the last point, the xml rule definition contains description and example, is there anything else to add?

@ghost

ghost commented Apr 3, 2021

Copy link
Copy Markdown
1 Message
📖 This changeset changes 0 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 2 errors and 2 configuration errors.
Full report
This changeset changes 0 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 2 errors and 2 configuration errors.
Full report

Generated by 🚫 Danger

@oowekyala oowekyala added this to the 6.34.0 milestone Apr 3, 2021
@oowekyala oowekyala changed the title #3190, UseStandardCharsets, java, new rule [java] New rule: UseStandardCharsets Apr 3, 2021

@oowekyala oowekyala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, this looks good! Please take a look at my comments to improve the details

Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
@aaime

aaime commented Apr 5, 2021

Copy link
Copy Markdown
Contributor Author

Changes have been applied, based on review. Should have addressed all points raised.

@aaime

aaime commented Apr 6, 2021

Copy link
Copy Markdown
Contributor Author

I'm not sure what to do of the pmd-test feedback though, seems to be raising issues about spring-framework somehow?
https://chunk.io/pmd/dfbf46e0331e46f0b41368ccf2a233f2/diff/index.html

Ah, the new check is finding two violations in Spring:

Working as intended it would seem? Does configuration need to be twaeked so that the check is not running by default out of the box?

@adangel

adangel commented Apr 6, 2021

Copy link
Copy Markdown
Member

Working as intended it would seem? Does configuration need to be twaeked so that the check is not running by default out of the box?

Looks good. Since this is a new rule, PMD finds now new violations, which is expected. The two new violations are valid cases for your new rule, so all is fine.
We have on the master branch all rules enabled by default, so that you directly can see the impact of your changes. No need for further configuration.

@oowekyala

Copy link
Copy Markdown
Member

I think this rule is a good candidate for the quickstart ruleset. @adangel wdyt?

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@adangel adangel self-assigned this Apr 10, 2021
@adangel adangel added the a:new-rule Proposal to add a new built-in rule label Apr 10, 2021
adangel added a commit that referenced this pull request Apr 10, 2021
adangel added a commit that referenced this pull request Apr 10, 2021
[java] New rule: UseStandardCharsets #3193
@adangel adangel merged commit 3b75c31 into pmd:master Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:new-rule Proposal to add a new built-in rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[java] Use StandardCharsets instead of Charset.forName

3 participants