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

Fix isReaderName for fields started with _ or $ #8435

Merged
merged 1 commit into from Dec 20, 2022

Conversation

altro3
Copy link
Contributor

@altro3 altro3 commented Nov 28, 2022

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2022

@graemerocher I think, this fix should to be in 3.8.0

@@ -369,7 +369,8 @@ public static boolean isReaderName(@NonNull String methodName, @NonNull String[]
}
int len = methodName.length();
if (len > prefixLength) {
isValid = Character.isUpperCase(methodName.charAt(prefixLength));
char firstVarNameChar = methodName.charAt(prefixLength);
isValid = firstVarNameChar == '_' || Character.isUpperCase(firstVarNameChar);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct way would be to use something like Character.isJavaIdentifierStart. And the tests should also include a case for $foobar

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 agree about $ symbol, but how you suggest to use Character.isJavaIdentifierStart ? Problem in symbol case isJavaIdentifierStart don't check letters case, but symbols _ and $ haven't case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dstepanov ping

Copy link
Contributor

Choose a reason for hiding this comment

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

According to JavaDoc: Determines if the character (Unicode code point) is permissible as the first character in a Java identifier. Does it pass tests if you replace isValid = Character.isJavaIdentifierStart(firstVarNameChar)?

Copy link
Contributor Author

@altro3 altro3 Dec 20, 2022

Choose a reason for hiding this comment

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

Determines if the specified character is permissible as the first character in a Java identifier.
A character may start a Java identifier if and only if one of the following conditions is true:
isLetter(ch) returns true
getType(ch) returns LETTER_NUMBER
ch is a currency symbol (such as '$')
ch is a connecting punctuation character (such as '_').

It means if you run this code:

        log.info("{}, {}, {}, {}",
                Character.isJavaIdentifierStart('A'),
                Character.isJavaIdentifierStart('a'),
                Character.isJavaIdentifierStart('$'),
                Character.isJavaIdentifierStart('_'));

you will see:

true, true, true, true

In other words, you can't figure out what case a given character is in. Yes, in theory you can capitalize an attribute, like so:

private String Field;

but getters and setters must be like this:

    public String getField() {
        return Field;
    }

    public void setField(String field) {
        Field = field;
    }

A lowercase letter in a getter can only occur in one case, if you name the variable like this:

private String mYfield;

In this case, you will get a classic naming problem that many people have - some systems think that after the word get and set there should be an uppercase letter, some think that there should be 2 capital letters. For example, lombok will generate methods like this:

    public String getMYfield() {
        return this.mYfield;
    }

    public void setMYfield(final String mYfield) {
        this.mYfield = mYfield;
    }

idea generate it like this:

    public String getmYfield() {
        return mYfield;
    }

    public void setmYfield(String mYfield) {
        this.mYfield = mYfield;
    }

How do we view this situation? If we allow that there can be a lowercase letter after the word get and set, then we need to add the condition that after this letter there must be an uppercase letter.

Or do you suggest completely abandoning the character case check and replacing the entire check with one method:

isValid =Character.isJavaIdentifierStart(firstVarNameChar);

in this case, absolutely all methods that start with the correct character after get will become valid. For example, method with name getmyget1223 - will be considered a valid getter. But, as we can see from my examples, that in correct getters and setters there is always at least one uppercase letter, except for the exception with the symbols _ and $:

// this is valid code and right getters and setters
    private String ___;
    private String $_$_;
    private String _a_;
    private String _A_;
    private String $B_;
    private String $B$;

    public String get___() {
        return ___;
    }

    public void set___(String ___) {
        this.___ = ___;
    }

    public String get$_$_() {
        return $_$_;
    }

    public void set$_$_(String $_$_) {
        this.$_$_ = $_$_;
    }

    public String get_a_() {
        return _a_;
    }

    public void set_a_(String _a_) {
        this._a_ = _a_;
    }

    public String get_A_() {
        return _A_;
    }

    public void set_A_(String _A_) {
        this._A_ = _A_;
    }

    public String get$B_() {
        return $B_;
    }

    public void set$B_(String $B_) {
        this.$B_ = $B_;
    }

    public String get$B$() {
        return $B$;
    }

    public void set$B$(String $B$) {
        this.$B$ = $B$;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that we can formulate 3 conditions for correctness:

  1. Starts with get and contains the characters _ or $
  2. Starts with get, does NOT contain the characters _ or $ and the next letter is capitalized.
  3. Starts with get, does NOT contain the characters _ or $ and the next letter in lower case, but the next letter must be in upper case.

Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks!

@altro3 altro3 changed the title Fix isReaderName for fields started with _ Fix isReaderName for fields started with _ or $ Nov 30, 2022
@sdelamo sdelamo merged commit 16b6e2a into micronaut-projects:3.8.x Dec 20, 2022
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.

None yet

3 participants