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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature PostConstructor #2522

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

Conversation

Rawi01
Copy link
Collaborator

@Rawi01 Rawi01 commented Jul 21, 2020

This PR adds @PostGeneratedConstructor. It is a simplified version of @PostConstructor(#1207) that simply ignores explicit written constructors. There are a bunch of open questions for the full version, thats why I started with this simplified version.

The name should make clear that it only modifies generated constructors. If at some point the full version gets implemented we can deprecate this one and replace the handler with a call to the new version.

I also added some test cases and the required parts for the website, the only thing thats missing is some feedback and a fancy short description for this feature 馃槃

@Rawi01
Copy link
Collaborator Author

Rawi01 commented Jul 23, 2020

I can also implement the full version if you prefer that one but we should decide how we handle explicit constructors first.

@rspilker
Copy link
Collaborator

rspilker commented Jul 23, 2020

I agree that fixing the case for generated constructors first is a good idea.

I have some open questions and remarks:

  • Wouldn't it make sense to allow static methods, possibly passing the this as first parameter?
  • Would it make sense to have allow for an explicit ordering if multiple methods are present?
  • I missed tests for multiple post-methods with overlapping exceptions.
  • The copyright notice for new files only need 2020 in there.

A bit more "out there": would it make sense to copy javadoc from the post-methods to the generated constructors?

The remarks and questions above are not requirements, I would just like to discuss their merits.

@Rawi01
Copy link
Collaborator Author

Rawi01 commented Jul 24, 2020

Thanks for the feedback 馃殌

Wouldn't it make sense to allow static methods, possibly passing the this as first parameter?

I also thought about static methods but could not find a real use case where it is better to create a static method instead of an instance method. In every case I evaluated it seems to be a design flaw to create a static method.

Would it make sense to have allow for an explicit ordering if multiple methods are present?

In situations like this I usually order my methods in the same order they get executed. But I agree that there might be cases where that is not possible e.g. enforced method ordering public > protected > private.

I missed tests for multiple post-methods with overlapping exceptions.

Ups, totally missed that. I will add one but IIRC you can define the same exception multiple times in the throws block so this should work out of the box. Do you think that duplicates should be removed?

The copyright notice for new files only need 2020 in there.

Copy paste ftw 馃槃

A bit more "out there": would it make sense to copy javadoc from the post-methods to the generated constructors?

Sounds like a great idea, I will check if that is possible.


I tried out how we could possibly modify explicit constructors and it seems like its not as hard as I thought. I created a visitor that scans the method for return statements and stops on lambdas and inner classes. For each return statements it reads the parent. If it is a block it simply adds the method call, otherwise it wraps the statement into a new block and adds the method call to this block. I commited the WIP here: https://github.com/Rawi01/lombok/tree/post-constructor-visitor

@Rawi01
Copy link
Collaborator Author

Rawi01 commented Jul 29, 2020

I have thought again about your questions and I think we should keep it as simple as possible. We can already handle these cases by adding a single @PostGeneratedConstructor method that calls other (static) methods in any order. Everything we add now requires maintainance. If it turns out that static or ordered methods are a common use case we can still add these features later.

About the javadoc, simply copying the comment works as long as there is only one source method. If there are multiple it always overrides the previous copy. This can be solved but I also noticed that we do not add comments to constructors and it looks really weird if the only comment of the constructor is something like Validates this object.

My proposal is that we:

  • add support for javadoc on constructors
  • then we add a method to append javadoc
  • and then we append the method javadoc to the constructor. Maybe a seperate section or heading might be reasonable.

@jayveeAtWork
Copy link

Would love to see the PR get merged into a future Lombok. In Spring JPA it's common (for bidirectional relationships) to set the current instance (this) in the object being added to a list of the current instance, which you of course can't do when the object isn't built yet.
This annotation would allow us to add a method which goes over the list of added objects and set "this" on them once Lombok has built the instance, but before returning it to the caller.

@trumpetinc
Copy link

Is there any status on this getting accepted? It is a major headache to have to manually rewrite large constructors (or build() methods - same amount of work) just to add validation. All other strategies (i.e. subclassing the generated builder) do not work cleanly with @Jacksonized, toBuilder(), etc...

@BreakBB
Copy link

BreakBB commented Apr 4, 2022

Are there any plans to merge this PR?

@Rawi01 Rawi01 changed the title Feature PostGeneratedConstructor Feature PostConstructor Apr 6, 2022
@AskMeAgain
Copy link

What is the state for this PR?

@baizhimafa
Copy link

Please help the child, I really need this feature馃

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

7 participants