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

Implement support for property builders #80

Merged
merged 15 commits into from
Nov 16, 2023
Merged

Implement support for property builders #80

merged 15 commits into from
Nov 16, 2023

Conversation

ZacSweers
Copy link
Collaborator

Resolves #79

@ZacSweers ZacSweers requested review from adamsp and stagg November 16, 2023 04:48
Copy link

@adamsp adamsp left a comment

Choose a reason for hiding this comment

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

I'm not sure how close to the original you want to fly here - what you have will work for existing implementations, but will behave differently than AutoValue if users make calls in the wrong order.

We're also missing the chained builder calls from the docs:

image

Edit: I suppose the remainingMethods TODOs will solve the chained builder calls, since those aren't generated anyway.

}

internal fun requiredBuildableCollection(`value`: ImmutableList<String>): Builder =
apply { this.requiredBuildableCollection = `value` }
Copy link

Choose a reason for hiding this comment

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

This implementation doesn't match the docs - I think we need to check if the member is null first:

The same caller can mix the two styles only in limited ways; once foosBuilder has been called, any subsequent call to setFoos will throw an unchecked exception. On the other hand, calling setFoos first is okay; a later call to foosBuilder will return a builder already populated with the previously-supplied elements.

Like this:

    @Override
    public FakeComment.Builder reactions(ImmutableList<Reaction> reactions) {
      if (reactions == null) {
        throw new NullPointerException("Null reactions");
      }
      if (reactionsBuilder$ != null) {
        throw new IllegalStateException("Cannot set reactions after calling reactionsBuilder()");
      }
      this.reactions = reactions;
      return this;
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oooo good catch, let me update

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ZacSweers
Copy link
Collaborator Author

for chained builders - isn't the doc saying to manually write those and just hide the abstract builder method?

@ZacSweers
Copy link
Collaborator Author

I think I need to update for this case as well

On the other hand, calling setFoos first is okay; a later call to foosBuilder will return a builder already populated with the previously-supplied elements.

Interestingly though, the generated code I see in AV doesn't appear to do this

image

@ZacSweers
Copy link
Collaborator Author

Ahhh it's only if a setter is present

image

@ZacSweers ZacSweers requested a review from adamsp November 16, 2023 18:15
@ZacSweers ZacSweers added this pull request to the merge queue Nov 16, 2023
Merged via the queue into main with commit 71d081a Nov 16, 2023
2 checks passed
@ZacSweers ZacSweers deleted the z/nestedBuilders branch November 16, 2023 20:21
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.

[FEATURE] Support auto value collection builders
2 participants