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.

Sign up for GitHub

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

Jump to bottom

Canonical constant naming #396

Open
1 of 2 tasks
rickie opened this issue Dec 7, 2022 · 8 comments · May be fixed by #794
Open
1 of 2 tasks

Canonical constant naming #396

rickie opened this issue Dec 7, 2022 · 8 comments · May be fixed by #794
Assignees
@mohamedsamehsalah
Labels
good first issue Good for newcomers new feature

Comments

@rickie
Copy link
Member

rickie commented Dec 7, 2022

Problem

According to the Google Java Format Style guide constants names should use UPPER_SNAKE_CASE.
There is a ConstantField check in Error Prone that does the following:

When naming a field with CONSTANT_CASE, make sure the field is static, final, and of immutable type. If the field doesn’t meet those criteria, use lowerCamelCase instead.

We want to go one step further and ensure that all constant names are UPPER_SNAKE_CASE.

Description of the proposed new feature

  • Support a stylistic preference.
  • Avoid a common gotcha, or potential problem.

I would like to rewrite the following code:

private static final int number = 1;
static final int otherNumber = 2;

to:

private static final int NUMBER = 1;
static final int OTHER_NUMBER = 2;

Considerations

Carefully read the GJF Style Guide on constant names and maybe use examples from there.

This check might require some additional analysis for static fields that are mutable. This should be a rare case though. Let's see how we handle this when we get there 😄.
@rickie rickie added good first issue Good for newcomers new feature labels Dec 7, 2022
@rickie rickie changed the title Canonical constants names Canonical constant naming Dec 7, 2022
@brunacunha
Copy link

Hello, I've noticed that to solve this issue, the only global variables that need this change are the private static final long serialVersionUID.

Is this issue still relevant? If it is, I would like to try to solve it. :)

@Stephan202
Copy link
Member

Hi @brunacunha! Good point about serialVersionUID; that's a special case which should be ignored by the hypothetical check described here, as it is part of Java's serialization functionality.

Looking at the issue description once more, we might also want to skip package-private fields, as those might be referenced by other classes. All things considered, the issue then calls for a check that:

  1. Renames private static final fields to UPPER_SNAKE_CASE.
  2. ... with the exception of serialVersionUID.

If you'd like to have a crack at writing such a BugChecker, then that's great! Feel free to ask us for advice/help when necessary, as this might not be an easy one.

A possible approach (there are likely others) is to:

  • Have the BugChecker identify "misnamed" fields.
  • In case of a hit, use a TreeScanner to find all IdentifierTrees in the surrounding CompilationUnit that represent the same symbol (using ASTHelpers.getSymbol(..)).
  • From the result, generate a suggested fix that renames all these references in one go. ✨

If this sounds rather daunting: we can also discuss some easier checks to get started with. 📈

@rickie
Copy link
Member Author

rickie commented Jan 11, 2023

Hi!

I've noticed that to solve this issue, the only global variables that need this change are the private static final long serialVersionUID. Is this issue still relevant?

Even if there would be no cases in Error Prone Support flagged by this BugChecker, still this issue would be relevant!
Within Picnic we apply all the checks on all of our internal Java codebases, where this check will definitely find and fix violations 😄.

@mohamedsamehsalah mohamedsamehsalah self-assigned this Sep 16, 2023
@mohamedsamehsalah mohamedsamehsalah linked a pull request Sep 17, 2023 that will close this issue
Introduce CanonicalConstantFieldName bug checker #794
Open
@BLasan
Copy link
Contributor

BLasan commented Jan 26, 2024

Hi @rickie ,

Is this issue still valid?

Thanks,
Benura

@rickie
Copy link
Member Author

rickie commented Jan 26, 2024

Hi @BLasan , yes it is!

@mohamedsamehsalah is working on it in #794.

You would like to work on a ticket of Error Prone Support or are you interested in this particular check?

@BLasan
Copy link
Contributor

BLasan commented Jan 27, 2024

Hi @BLasan , yes it is!

@mohamedsamehsalah is working on it in #794.

You would like to work on a ticket of Error Prone Support or are you interested in this particular check?

Yes @rickie . I would like to work on a ticket of Error Prone Support :) If there's any newbie issue (or any) please let me know. Would be happy to work on it

@rickie
Copy link
Member Author

rickie commented Jan 28, 2024

Hi @BLasan , I created a ticket that is a nice introduction to get started with Refaster: #1001.

I'd say, give it a go and let us know if you have any questions or run into any kind of problems.

@BLasan
Copy link
Contributor

BLasan commented Jan 29, 2024

Hi @rickie ,

Will give it a try. Thanks for creating one :)

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

@mohamedsamehsalah mohamedsamehsalah

Labels
good first issue Good for newcomers new feature
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Introduce CanonicalConstantFieldName bug checker PicnicSupermarket/error-prone-support
5 participants
@Stephan202 @rickie @brunacunha @BLasan @mohamedsamehsalah