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

Change lombok's property capitalization code. Field xName -> getxName(). #2693

Closed
rzwitserloot opened this issue Jan 2, 2021 · 16 comments · Fixed by #2996
Closed

Change lombok's property capitalization code. Field xName -> getxName(). #2693

rzwitserloot opened this issue Jan 2, 2021 · 16 comments · Fixed by #2996
Assignees
Milestone

Comments

@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Jan 2, 2021

This is a catch-all issue that tries to capture the relevant information from a ton of separate issue requests (to those reviewing bug reports, please mark as duplicate of just this issue).

The problem:

Lombok's name conversion tool, used by @Getter and @Setter, as well as @With, and therefore also @Data and @Value, turns a field that starts with a single lowercase letter followed by a capital into a capital followed by a capital: @Getter private String uName; generated public String getUName() {return this.uName; }.

However, various tools out there expect getuName instead, in this unique case where the second letter of the field/property name is a capital where the first is lowercase.

Unfortunately, there is no convention we can follow, because there are also tools out there that expected getUName. No matter what lombok does, some tooling / expectations will be left out in the cold. More generally then, because of these conflicts, names like uName are problematic in any case, and you should try never to use such names.

There is nothing lombok can do to solve all problems with single-lower-case-letter property names: If you decide to use fields named like this, sooner or later you're going to run into trouble. Quoting user @Maaartinus:

Javabeans is fubared. Some long time ago I was unhappy about Lombok mistreating my "uShaped" property and wrote manual etters as a workaround. IIRC it didn't work either, because some other tool didn't understand it in the opposite direction (setuShaped -> uShaped), so I ended using "shapedAsU" instead. 🙄

However, the majority of the community has standardized on xName -> getxName(), and thus we are now willing to accept a PR, even though this will cause no small amount of pain, as it'll be backwards incompatible, and we reserve the right to wimp out and delay integrating the Pull Request until a major update with more breaking changes.

Tools and specs that prefer xName -> getxName:

[context: This describes how to derive a property name from the existence of getX and/or setX methods; it goes from getter to property name, which is the reverse of what lombok does]: ... when we extract a property or event name from the middle of an existing Java name, we normally convert the first character to lower case. However, to support the occasional use of all upper-case names, we check if the first two characters of the name are both upper case and if so leave it alone. So for example, FooBahfooBah, Zz, URLURL.

(warning from @rzwitserloot: Do not write setURL. All major style guides and modern JDK standards say you should write setUrl. This nuance wasn't around in 1997 / it was, but this document tries to cater to existing code that did not follow it; a bunch of JDK methods, particularly in swing, fail to adhere to this, for example).

(It's subtle, but this URLURL rule means that getXName() is turned into property name XName because it starts with 2 capitals. This is presumably the source of why xName -> getxName, because getXName doesn't convert back to xName according to beanspec §8.8.

  • JSP
  • IntelliJ's code generation ('generate getter / setter')
  • Eclipse's code generation ('generate getter / setter')
  • java.beans.Introspector's decapitalize method.

Tools and specs that prefer xName -> getXName:

Tools that used to xName -> getXName but now xName -> getxName:

See also this Stack overflow answer that goes into a lot of depth on this sad state of affairs, as well as this conversation on the lombok list.

Workaround:

Just write the getter/setter out, with the casing you want. Lombok will see it and recognize that you clearly wanted this alternate casing:

@Getter private String xName;

// write out the getter explicitly:
public String getxName() { ... }
// .. and lombok will not generate getXName.

This issue is a catch-all for at least 25 earlier issues; all relevant information from all issues marked as duplicate is included in this one post.

This was referenced Jan 2, 2021
@maxxyme
Copy link

maxxyme commented Jan 4, 2021

is there a way to fix the (missing) link for ... to the other behaviour (SOURCE: this issue.? thanks.

@rldeep2889

This comment has been minimized.

@rzwitserloot
Copy link
Collaborator Author

@maxxyme wrote: is there a way to fix the (missing) link for ... to the other behaviour (SOURCE: this issue.? thanks.

Fixed.

@SimSonic
Copy link

SimSonic commented Aug 20, 2021

IDK if this relates to current issue but I just keep it here to not create more requests.

Code:

@lombok.Value
public class SrcDto {
    String filename;
    String fileName;
}

results in only one getter:


public final class SrcDto {
    private final String filename;
    private final String fileName;

    public SrcDto(String filename, String fileName) {
        this.filename = filename;
        this.fileName = fileName;
    }

    public String getFilename() {
        return this.filename;
    }

    public boolean equals(Object o) {
        if (o == this) {
            return true;
        } else if (!(o instanceof SrcDto)) {
            return false;
        } else {
            SrcDto other = (SrcDto)o;
            Object this$filename = this.getFilename();
            Object other$filename = other.getFilename();
            if (this$filename == null) {
                if (other$filename != null) {
                    return false;
                }
            } else if (!this$filename.equals(other$filename)) {
                return false;
            }

            Object this$fileName = this.getFilename();
            Object other$fileName = other.getFilename();
            if (this$fileName == null) {
                if (other$fileName != null) {
                    return false;
                }
            } else if (!this$fileName.equals(other$fileName)) {
                return false;
            }

            return true;
        }
    }

    public int hashCode() {
        int PRIME = true;
        int result = 1;
        Object $filename = this.getFilename();
        int result = result * 59 + ($filename == null ? 43 : $filename.hashCode());
        Object $fileName = this.getFilename();
        result = result * 59 + ($fileName == null ? 43 : $fileName.hashCode());
        return result;
    }

    public String toString() {
        String var10000 = this.getFilename();
        return "SrcDto(filename=" + var10000 + ", fileName=" + this.getFilename() + ")";
    }
}

So here we can see only one getter and it's misuse in other methods.

Lombok 1.18.20

@SimSonic
Copy link

@rzwitserloot any news?

@YonatanSherwin
Copy link

@rzwitserloot what about adding optional configuration, something like JavaBeansSpecCapitalization=true (default to false)?
I'll be happy to take that if you think it's a good idea

@rzwitserloot
Copy link
Collaborator Author

@SimSonic That is another one of those "Whatever we do, 90% don't care, though have a mild preference for no changes to the status quo whatever it may be, 5% want A, and 5% want B". We chose A, you want B, even if we could go back in time and argue that B is slightly better, now that we are here, no change wins due to the 90%. We aren't fixing that. Why do you have 2 fields with the exact same name other than capitalization?

@rzwitserloot
Copy link
Collaborator Author

@YonatanSherwin I think using lombok.config for this is the right place, yes. I'll ask @rspilker tomorrow, as this is a bit of a touchy subject :) Stay tuned to this issue :P

@SimSonic
Copy link

SimSonic commented Oct 14, 2021

Why do you have 2 fields with the exact same name other than capitalization?

True story, gateway between mobiles and fintech services, which renames the field accidentally... I've tried to read both name variants and select non-null one.

Ok, I'm agree, it's a stupidly rare case.

But it's hard for me look at current behavior as "not a bug, feature". I'm understanding your position and just want you to not discard possibility of future bugfix.

Thanks for you work.

@YonatanSherwin
Copy link

Thanks @rzwitserloot , waiting for update. I'll be happy to implement this if that's ok :-)

@rzwitserloot
Copy link
Collaborator Author

@YonatanSherwin:

@rspilker wasn't feeling so good. I don't see any reason for not doing this, but it's been a contentious issue so I'm sure he has thoughts on the topic. We'll try to discuss it this weekend.

@rspilker
Copy link
Collaborator

I'm all for accepting a pull request that caters both capitalization strategies based on a config key. I prefer the default to be the current strategy in order to not introduce a breaking change.

@YonatanSherwin
Copy link

PR link:
#2996

@rzwitserloot
Copy link
Collaborator Author

The current edge includes this update (the changelog listed on the edge download page is wrong, it's in there):

https://projectlombok.org/download-edge

sadv1r pushed a commit to sadv1r/lombok that referenced this issue Nov 23, 2021
sadv1r pushed a commit to sadv1r/lombok that referenced this issue Nov 23, 2021
@maxxyme
Copy link

maxxyme commented Nov 25, 2021

The current edge includes this update (the changelog listed on the edge download page is wrong, it's in there):

https://projectlombok.org/download-edge

Do you plan to mention the default value basic somewhere soon? [Indeed it's not on the "edge" changelog announcement]
And I had to browse through the last commit to find out how it was updated (originally in the PR it was a true/false flag...)
Thank you.

@rzwitserloot
Copy link
Collaborator Author

@maxxyme It's in the docs, but, as this version hasn't been rolled out yet, the docs that mention it also haven't been rolled out yet. See commit 13d84b1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants