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

Alternative name for build method #432

Closed
neilswingler opened this issue Jul 28, 2020 · 9 comments · Fixed by #438
Closed

Alternative name for build method #432

neilswingler opened this issue Jul 28, 2020 · 9 comments · Fixed by #438

Comments

@neilswingler
Copy link

It could be useful, especially when migrating legacy hand coded builders to Freebuilder, to be able to have the build() method have a different name.

e.g. by declaring the alternative name in the Builder

public Foo buildFoo() {
  return super.buildFoo();
}
@alicederyn
Copy link
Collaborator

alicederyn commented Jul 28, 2020

Is it necessary to not have the build method present? Otherwise you could just declare the legacy method in addition:

public Foo buildFoo() {
  return build();
}

@neilswingler
Copy link
Author

neilswingler commented Jul 29, 2020

In my use case I want to overload the actual build() method to return a different type so I require the generated builder to not declare a build() method at all.

I have a mutable data object with getters/setters and a hand coded builder that I hope will eventually be migrated to an immutable Freebuilder implementation but as a stepping stone I want to at least be able to use FreeBuilder for the builder part.

@alicederyn
Copy link
Collaborator

Can you post a simplified example of what that API would look like?

There are two methods on builders with the same signature (no parameters, returns a Foo), build and buildPartial, and while I can't think of any others users might want to add, it would be API-breaking to stop emitting build if one were added — but depending on the exact pattern you're making, it may still be possible to reliably detect it without breaking other use cases.

@neilswingler
Copy link
Author

Could this work? It would not be a breaking change because currently, overloading the build() method return type leads to a compiler error.

@Freebuilder
interface Foo {
    class Builder extends Foo_Builder {
        public Bar build() { ... }  // incompatible return type => don't emit a build()
        public Foo buildFoo() { // compatible return type, emit buildFoo instead
            return super.buildFoo();
        |
    }
}

@alicederyn
Copy link
Collaborator

I would prefer to find a non-obscured method name matching a specific pattern for selecting the build method — perhaps this:

  1. If the build() method is protected or package-protected on the subclass, follow suit
  2. If the build() method is private or signature-incompatible, instead generate a package-protected _buildImpl() method for the subclass to call
  3. Optional for first implementation If that method is already present and incompatible, continue trying with _buildImpl2() etc. until a suitable candidate is found

This then cleanly extends to other methods in future, e.g. buildPartial() becomes _buildPartialImpl()

That would make your code look like:

@FreeBuilder
interface Foo {
    class Builder extends Foo_Builder {
        public Bar build() { ... }  // incompatible return type => FreeBuilder emits _buildImpl instead
        public Foo buildFoo() {
            return super._buildImpl();
        }
    }
}

I would be happy to accept a PR implementing this feature. If you are interested in doing this, let me know and I'll point you to the bits of the code you'd need to fit it into.

@neilswingler
Copy link
Author

neilswingler commented Jul 31, 2020 via email

@alicederyn
Copy link
Collaborator

Gmail doesn't notify me of forum messages, so if you do pick it up, feel free reach out to me directly by email for a quicker response. Meanwhile, enjoy your holiday!

@alicederyn alicederyn changed the title Feature request: Aternative name to build() Alternative name for build method Aug 3, 2020
@alicederyn
Copy link
Collaborator

Hopefully FreeBuilder v2.7.0 will solve this issue for you. Please do raise another issue if it does not!

@neilswingler
Copy link
Author

Thanks a lot. I tried it out already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants