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

Record support #840

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Record support #840

wants to merge 4 commits into from

Conversation

liach
Copy link

@liach liach commented May 13, 2021

Fixes #829

A short summary:
Added record support to javapoet as it has become a stable feature in modern java. Turns out the support is quite easy given the robust existing framework in javapoet.

  1. Shared some code from method spec for emitting parameter and their javadocs for record components.
  2. Moved the emission of type headers to a separate method since otherwise the type emission method would be too long (per checkstyle)
  3. Added a few tests ensuring the functionalities for basic record creation.

mvn clean verify ends with 17 file-ending errors (presumably because of crlf/lf problems), which I cannot fix locally; all tests pass. CLA signed as well.

It's my pleasure to have you reviewing my code.

P.S. for more information about java records, feel free to check out the related java language specification section and look at a jdk example and its javadocs

Fixes square#829

Signed-off-by: liach <[email protected]>
@Randgalt
Copy link
Contributor

Hi @JakeWharton - can you possibly let the community know the plans for JavaPoet and support for post Java 11 features? Will this PR or something similar be committed to the codebase soon? Thank you.

@JakeWharton
Copy link
Member

I no longer participate in the day-to-day of this repository or have the bandwidth to review changes with such large API surface. Hopefully @Egorand or someone will pick it up.

@Randgalt
Copy link
Contributor

If Square isn't going to continue with Javapoet maybe we can fork it and continue supporting it. I'd be happy to host/republish it. Let's see what @Egorand says.

@JakeWharton
Copy link
Member

You are always free to do so, but don't mistake my lack of involvement which has been consistent for the last 5 years as any larger indicator. I was mentioned personally therefore I commented on my personal involvement.

@Randgalt
Copy link
Contributor

Thanks @JakeWharton

@tanadeau
Copy link

Any updates on this? This is one of the exciting features of Java 17 since it's now been released.

@liach
Copy link
Author

liach commented Sep 22, 2021

@Egorand Mind review this, or is javapoet of low priority right now?

@Egorand
Copy link
Collaborator

Egorand commented Sep 23, 2021

Apologies for not replying sooner and thanks for your contribution! Unfortunately I lost context on this project and Java in general a while ago and can't review this efficiently at the moment. I'll try to find someone who can, or hopefully find some time in the near term to catch up.

@arouel
Copy link

arouel commented Nov 15, 2021

I reviewed the code and I think it is good to go. It doesn't break the existing API nor logic and introduces Record support.

I'm also in for forking this project, if there is no interest or time from Square anymore to maintain this project.

@fcurts
Copy link

fcurts commented Nov 17, 2021

Is there any good alternative to JavaPoet that supports generating records?

@Randgalt
Copy link
Contributor

If we do fork, what needs to be done? I have a Maven Group ID prefix we can use and I can already publish to Maven Central with it. I think we'd have to change the project's group ID at minimum. Do we have to change the license? Copyright? Package names? Anyone know?

@gzsombor
Copy link

You should not change the license or copyright, that would be illegal.
When I fork a project, mostly for personal usage, I just use jitpack.io, which can build any Maven/Gradle projects and distribute the built jars in a coherent way, for example you can use javapoet from there with:
https://jitpack.io/#square/javapoet/javawriter-2.5.1

@Randgalt
Copy link
Contributor

The difference here is that I would like to re-publish this so others can use/contribute. That's allow under AL2 right?

@fcurts
Copy link

fcurts commented Nov 17, 2021

Changing the license isn't illegal but shouldn't be necessary. I wonder if the package name should be changed though.

Publishing to Maven Central is more trustworthy than jitpack (which is denylisted by many organizations).

In my mind, the main question is whether someone would be willing/able to maintain a fork in the long run.

@gzsombor
Copy link

You can only modify the license, if you were the original author / copyright holder. Probably in javapoet case, it would mean if all the contributor agree on a license change. But I don't think license change is needed, AL2 is a really permissive license, you can freely modify, publish,etc.
Sure, publishing to Maven Central makes the code more trustworthy, and makes it more easy to use.

@liach
Copy link
Author

liach commented Nov 17, 2021

Fyi I have a fork including this content, available over github packages https://github.com/liachmodded/javapoet

@fcurts
Copy link

fcurts commented Nov 17, 2021

You can only modify the license, if you were the original author / copyright holder.

I don't think this is true (https://en.wikipedia.org/wiki/Apache_License#Licensing_conditions). But as you said, there's no need to change the license.

@JakeWharton
Copy link
Member

Can you add a test with a secondary constructor?

Something like

record Person(String name) {
    Person(int number) {
        this.name = Integer.toString(number);
    }
}

@liach
Copy link
Author

liach commented Nov 18, 2021

yeah, will do later today

@JakeWharton
Copy link
Member

Looks good otherwise. I'll have another read of the JLS just to make sure we're covering our bases.

@liach
Copy link
Author

liach commented Nov 18, 2021

Done. CI results available at https://github.com/liachmodded/javapoet/runs/4255899160 (The java 8 one fails because setup java action cannot find java 8 by 1.8)

@JakeWharton
Copy link
Member

I thought about this over the weekend more.

I'm not sure it makes sense to put the record parameters directly on the type. What do you think about instead changing compactConstructor to recordConstructor and pulling the parameter specs from that? It also means that we do not need to place the awkward varargs property on a type.

@liach
Copy link
Author

liach commented Nov 22, 2021

Such a setup may be feasible, but compared to the current impl, there are a few behaviors to be defined:

  1. Currently, two sets of parameter specs actually differ for the javadoc they respectively contribute to the compact constructor/class. Need to check in javac on what the behavior of compact constructor parameter docs are like.
  2. Think a compact constructor with an empty body is legal as well, and we might want to distinguish that from the absence of a compact constructor.

@fcurts
Copy link

fcurts commented Dec 7, 2021

How can we get this unblocked? Do the maintainers intend to continue this project?

@marceloverdijk
Copy link

I was looking for this as well...
Would be nice if Square would let the community know what they intend to do this with the library; last commit is from 9 months ago so seems to have at least stalled...

@swankjesse
Copy link
Member

Update from Square

Apologies for radio silence on this. We're busy!

I'm talking to our internal copyright & IP legal team to hand off the project. There's logistics here to figure out how & who. But the plan is to hand-off JavaPoet to folks that can give it the attention it deserves.

@marceloverdijk
Copy link

@swankjesse Thx for the clarification.

@fcurts
Copy link

fcurts commented Feb 9, 2022

Another month has passed. Are they any news on handing off the project?

@JakeWharton
Copy link
Member

Handing off the project is orthogonal to this PR. And this is not a good place to discuss it. There's a discussions tab. Please use that from now on for project-wide discussion topics.

As to this PR, I expressed concern with the existing shape of the API design so it will not be merged as-is. Either the changes need made to this PR or a new PR needs opened.

We are not working on this project so we no one from our side will be making the changes required to land this support. That is unfortunate, but placing this project into the open is not a promise that we would work on it forever. It exists in open source for you to use, or not use, free of charge. If you are blocked and need this API, forking the project and adding support through this PR's diff or your own design is not that hard to do. You will have to maintain that fork going forward, but doing so is orders of magnitude more trivial than having to write an entire Java code generation API in the first place. There are surely other libraries or template engines that you could use as well.

Please keep future discussions on this PR about the design of the API in the diff.

@tanadeau
Copy link

tanadeau commented Feb 9, 2022

@JakeWharton: There is not a discussions tab turned on for this repo.

@JakeWharton
Copy link
Member

Ah, thanks for the heads up! The tab was enabled but apparently I had to do a one-time setup thing (which I do not recall doing on other repos).

@fcurts
Copy link

fcurts commented Feb 9, 2022

DIscussion: #866

iamdanfox added a commit to palantir/conjure-java that referenced this pull request Jul 12, 2022
iamdanfox added a commit to palantir/conjure-java that referenced this pull request Jul 12, 2022
iamdanfox added a commit to palantir/conjure-java that referenced this pull request Jul 12, 2022
cassandrus added a commit to deplant/javapoet that referenced this pull request Feb 1, 2023
- square#934
- square#925
- square#913
- square#840
- more support for records
- slight fixes
@pawellabaj
Copy link

Is there anyone who can tell what's the state of this PR?
Is there a chance to merge it?

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.

Add support for Java 16 records