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

Remove DefaultLocale in favor of StringCaseLocaleUsage #2589

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pkoenig10
Copy link
Member

This check is now provided by Error Prone, so we no longer need our custom DefaultLocale check.

Comment on lines -136 to -138
// StringCaseLocaleUsage duplicates our existing DefaultLocale check which is already
// enforced in some places.
"StringCaseLocaleUsage",
Copy link
Member Author

Choose a reason for hiding this comment

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

This effectively changes this check from SUGGESTION to WARNING, but we've had an excavator for this for a while now, so this should be acceptable.

Comment on lines -136 to -138
// StringCaseLocaleUsage duplicates our existing DefaultLocale check which is already
// enforced in some places.
"StringCaseLocaleUsage",
Copy link
Member Author

Choose a reason for hiding this comment

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

I see 5 references to DefaultLocale in our repos:

  • 3 are increasing the severity to WARNING or ERROR, so this change is fine for those repos
  • 2 are disabling this check, so those 2 repos will need to replace DefaultLocale with StringCaseLocaleUsage

Copy link
Contributor

Choose a reason for hiding this comment

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

so this change is fine for those repos

I believe compilation will fail because no check exists with name DefaultLocale after this PR merges

Copy link
Member Author

Choose a reason for hiding this comment

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

Here are the PRs for 2 of those 3. I'll also put up a PR for the one internal repo:

@pkoenig10
Copy link
Member Author

@carterkozak this should be good to merge now

@pkoenig10
Copy link
Member Author

@carterkozak any other concerns here?

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

2 participants