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

Ban the var keyword via error-prone VarUsage #1788

Merged
merged 2 commits into from
Jun 14, 2021
Merged

Conversation

carterkozak
Copy link
Contributor

@carterkozak carterkozak commented Jun 14, 2021

Before this PR

var?

After this PR

This will automatically migrate existing uses to the
implementation type used by the var keyword.

==COMMIT_MSG==
Ban the var keyword via error-prone VarUsage
==COMMIT_MSG==

Possible downsides?

Folks might use and enjoy the var keyword.

I didn't update the readme to enumerate the new error-prone rule. It's already missing a few, and we should borrow the upstream html generator instead.

This will automatically migrate existing uses to the
implementation type used by the `var` keyword.
@changelog-app
Copy link

changelog-app bot commented Jun 14, 2021

Generate changelog in changelog/@unreleased

Type

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

Description

Ban the var keyword via error-prone VarUsage

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from jkozlowski June 14, 2021 19:47
.addFix(fix.replace(
token.pos(),
token.endPos(),
SuggestedFixes.prettyType(state, fix, ASTHelpers.getType(typeTree)))
Copy link
Contributor

Choose a reason for hiding this comment

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

this will suggest using the concrete type instead of the interface which is typically prefered. It would be a ton of work to find the minimum type required of the variable though so its probably fine

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we just force the human to input the right type instead of a fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to give them a starting point, the reviewer is responsible for ensuring the quality of the replacement. If the replacement quality is too low we can remove the suggestion.

If we don't automate any of it, they're more likely to turn off the check instead of fixing it themselves.

@ferozco
Copy link
Contributor

ferozco commented Jun 14, 2021

👍

@bulldozer-bot bulldozer-bot bot merged commit 770175c into develop Jun 14, 2021
@bulldozer-bot bulldozer-bot bot deleted the ckozak/ban_var branch June 14, 2021 19:55
@svc-autorelease
Copy link
Collaborator

Released 3.91.0

This was referenced Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants