error-prone-support icon indicating copy to clipboard operation
error-prone-support copied to clipboard

Canonical constant naming

Open rickie opened this issue 1 year ago • 8 comments

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

  • [x] 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 avatar Dec 07 '22 11:12 rickie

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. :)

brunacunha avatar Jan 10 '23 22:01 brunacunha

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. :sparkles:

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

Stephan202 avatar Jan 11 '23 05:01 Stephan202

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 😄.

rickie avatar Jan 11 '23 07:01 rickie

Hi @rickie ,

Is this issue still valid?

Thanks, Benura

BLasan avatar Jan 26 '24 15:01 BLasan

Hi @BLasan , yes it is!

@mohamedsamehsalah is working on it in https://github.com/PicnicSupermarket/error-prone-support/pull/794.

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

rickie avatar Jan 26 '24 21:01 rickie

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

BLasan avatar Jan 27 '24 13:01 BLasan

Hi @BLasan , I created a ticket that is a nice introduction to get started with Refaster: https://github.com/PicnicSupermarket/error-prone-support/issues/1001.

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

rickie avatar Jan 28 '24 12:01 rickie

Hi @rickie ,

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

BLasan avatar Jan 29 '24 05:01 BLasan