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

[Feature] staticConstructor should use already defined private constr… #2119

Conversation

daliclass
Copy link
Contributor

@daliclass daliclass commented May 5, 2019

…uctor if available

to solve issue #2100

@rspilker
Copy link
Collaborator

rspilker commented May 6, 2019

Thanks a lot!

To be legally covered, can you please add your name to the AUTHORS file and adjust the pull request?

I'm still reviewing the code, looks good so far.

Great that you included the tests for both delombok and ecj!

staticConstrRequired ? AccessLevel.PRIVATE : level, typeNode, fieldsToParam, forceDefaults,
sourceNode, onConstructor);
injectMethod(typeNode, constr);
if (skipIfConstructorExists != SkipIfConstructorExists.NO && constructorExists(typeNode) != MemberExistsResult.NOT_EXISTS) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why

if (condition) {
  // generateStatic()
} else {
  // createConstructor
  // generateStatic()
}

instead of

if (!condition) {
  // createConstructor
}
//generateStatic()

I think the latter is simpler.

Copy link
Contributor Author

@daliclass daliclass May 6, 2019

Choose a reason for hiding this comment

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

Fixed good catch 👍

@rspilker
Copy link
Collaborator

rspilker commented May 6, 2019

I've added a comment in the code. This comment applies for both Eclipse and Javac.

@daliclass daliclass force-pushed the feature_value_annotation_should_use_already_defined_private_constructor branch from 635fc26 to 09ed5a8 Compare May 6, 2019 22:16
@daliclass
Copy link
Contributor Author

daliclass commented May 6, 2019

Thanks a lot!

To be legally covered, can you please add your name to the AUTHORS file and adjust the pull request?

I'm still reviewing the code, looks good so far.

Great that you included the tests for both delombok and ecj!

Added to Authors, gotta love tests 👍

@rspilker rspilker merged commit d3ae4d9 into projectlombok:master May 7, 2019
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

2 participants