-
Notifications
You must be signed in to change notification settings - Fork 9
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 incomplete toBuilder
implementations and add tests
#85
base: develop
Are you sure you want to change the base?
Conversation
Thanks for your interest in palantir/javapoet, @Marcono1234! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
toBuilder
methods and add teststoBuilder
implementations and add tests
Note: Unless you have a policy to create a release after every change, I would not mind if you wait a bit. I might create another PR as well. (And these fixes are not that critical that they require immediate releases?) |
javapoet/src/test/java/com/palantir/javapoet/FieldSpecTest.java
Outdated
Show resolved
Hide resolved
@@ -205,8 +205,7 @@ public Builder addAnnotation(Class<?> annotation) { | |||
} | |||
|
|||
public Builder addModifiers(Modifier... modifiers) { | |||
Collections.addAll(this.modifiers, modifiers); | |||
return this; | |||
return addModifiers(Arrays.asList(modifiers)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this is a sneaky one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: The FieldSpec
and TypeSpec
builder currently have a varargs variant only (and no Iterable
one), I am a bit afraid the same issue could re-appear there again in case an Iterable
overload is added as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like to add the Iterable
overload for symmetry in this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have included that in #87, I hope that is ok. I will probably revert this addModifiers
change here.
Our robots make releases trivial -- we generally release from all human-authored commits, releasing one change doesn't make it any less likely we'll cut a release on another PR :-] |
Yeah, my concern wasn't so much how easy it is to make a release 😅 I just wanted to express that I am not urgently waiting for these changes to be released. |
- For JavaFile it was not copying `staticImports` - For TypeSpec it was not copying `recordConstructor`
c27edaa
to
8ac376e
Compare
This change looks good, please sign the cla
This change looks good, thanks! Please complete the cla: #85 (comment) |
Currently still waiting for an answer to my questions regarding the CLA, see #84 (comment). |
Description
staticImports
recordConstructor
originatingElements
from nested types for everytoBuilder().build()
cyclejavadoc