Skip to content
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

WIP Avoid new HashMap(int) #1771

Conversation

davidscottcohen
Copy link
Contributor

Before this PR

HashMap(int) has confusing behavior, use Maps.newHashMapWithExpectedSize(int) instead.

After this PR

==COMMIT_MSG==
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented May 21, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

WIP Avoid new HashMap(int)

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from CRogers May 21, 2021 00:41
+ ".newHashMapWithExpectedSize which behaves as expected."
+ "See"
+ " https://github.com/palantir/gradle-baseline/blob/develop/docs/best-practices/java-coding-guidelines/readme.md#avoid-generics-clutter-where-possible"
+ " for more information.")
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move this message to the @BugPattern annotation summary, and it will be applied to the fix automatically when we don't call setMessage.


SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
String newType = SuggestedFixes.qualifyType(state, fixBuilder, Maps.class.getName());
String arg = tree.getArguments().isEmpty()
Copy link
Contributor

Choose a reason for hiding this comment

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

isEmpty should never evaluate to true for a constructor which takes an int argument -- we can either throw, or return Description.NO_MATCH. I tend to prefer the latter because we don't want to fail build sin cases that we haven't considered or don't understand.


@AutoService(BugChecker.class)
@BugPattern(
name = "AvoidNewHashMapInt",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this to the list of checks we automatically fix in BaselineErrorProneExtension, that way humans only have to review the change.

@carterkozak
Copy link
Contributor

The changelog robot doesn't know how to push to forks, you'll need to add the changelog manually.

Looks like the formatter needs to run as well, you can install the plugin for IntelliJ to update on save, https://plugins.jetbrains.com/plugin/13180-palantir-java-format, or run ./gradlew format

@davidscottcohen
Copy link
Contributor Author

Closing in favor of branch on the main repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants