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

Generate builder #460

Closed
wants to merge 28 commits into from
Closed

Generate builder #460

wants to merge 28 commits into from

Conversation

drewhamilton
Copy link
Owner

@drewhamilton drewhamilton commented Dec 23, 2024

Closes #304.

Uses FIR declaration generation to create a nested Builder class to classes annotated with @PokoBuilder.

To-do:

  • Generate factory function
  • Respect literal default values
  • Respect more complex default values
  • Generate copy function
  • Enforce only properties in constructor
  • Handle existing declarations that clash with generated Builder
  • Decouple from @Poko annotation
  • Decide what configuration to support

Comment on lines +19 to +23
builder
.setInt(1)
.setRequiredString("two")
.setOptionalString(null)
.setOptionalString("three")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if we should let the builder die, and instead focus on mutable mirrors of the immutable types with some nice functions. Things like val user: User = User.build { .. } plus relying on Kotlin scoping functions like .apply { } as you're doing below.

Copy link
Owner Author

@drewhamilton drewhamilton Jan 8, 2025

Choose a reason for hiding this comment

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

I think you're referring to the factory function which I hadn't added yet at the time I opened this? But that doesn't require dropping the builder setters so maybe you mean something more substantially different.

In any case I'm closing this in favor of bnorm/piecemeal which was already way ahead of me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think generating builders are really a good idea. I would prefer to see the ability to generate mutable versions of the main poko type.

Copy link
Owner Author

Choose a reason for hiding this comment

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

There's a few obvious differences (class name, this-returning setters) and a lot of similarities between the two. What's the important distinction that makes one a good idea and the other one not so good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A builder works around Java language deficiencies, and Kotlin doesn't need those workarounds. We don't need named functions because we have named arguments and named properties. We don't need chaining because we have .apply.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok I think we're on the same page then. I'll see if Brian will consider turning the setter functions off by default in piecemeal.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@ZacSweers
Copy link

FYI I think what you're trying to build here already exists: https://github.com/bnorm/piecemeal

IR body not generated yet.
@drewhamilton
Copy link
Owner Author

Noted, thanks for pointing that out. I reached out to Brian and I likely won't merge this if he plans to release that.

@drewhamilton
Copy link
Owner Author

Closing this in favor of bnorm/piecemeal. Once that's released I'll add tests to ensure Poko and piecemeal work well together.

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 the ability to generate a builder
3 participants