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

Renaming nested builders #1435

Open
perceptron8 opened this issue Jan 17, 2023 · 2 comments
Open

Renaming nested builders #1435

perceptron8 opened this issue Jan 17, 2023 · 2 comments
Labels
Component: value P3 type=enhancement Make an existing feature better

Comments

@perceptron8
Copy link
Contributor

Hi! I'm looking for a way to declare / use nested property builders with names other than default, but can't make it work. I've read https://github.com/google/auto/blob/main/value/userguide/builders-howto.md#accumulate, yet found nothing related.

@AutoValue
public abstract class Animal {
	public abstract String name();
	public abstract int numberOfLegs();
	public abstract ImmutableSet<String> countries();
	
	public static Builder builder() {
		return new AutoValue_Animal.Builder();
	}
	
	@AutoValue.Builder
	public abstract static class Builder {
		public abstract Builder setName(String value);
		public abstract Builder setNumberOfLegs(int value);
		public abstract ImmutableSet.Builder<String> countriesAccumulator(); // <- this doesn't work
		public abstract Animal build();
	}
}

Possibility to use anything other than fooBuilder() would be great. This probably means that logic would have to be based entirely on method's return type. This seems to be the way it works in the case of public static Builder builder() which can be renamed freely (https://github.com/google/auto/blob/main/value/userguide/builders-howto.md#-use-different-names-besides-builderbuilderbuild).

Ideally, I would like to be able to declare public abstract ImmutableSet.Builder<String> countries() with no suffix at all. Currently it leads to name collision with countries getter (as [AutoValueBuilderReturnType] nicely explains), but I hope that it is still possible to extend the implementation in a backward compatible way. Distinction between getters and builders seems to be rather unambiguous.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Jan 17, 2023

I don't think using the method's return type alone would work in general. What if you had more than one property of type ImmutableSet<String>? We could imagine accepting any suffix on the end of the property name, including the empty suffix, so all these would work:

abstract ImmutableSet.Builder<String> countriesBuilder(); // the thing that works today
abstract ImmutableSet.Builder<String> countriesAccumulator();
abstract ImmutableSet.Builder<String> countries();

That's potentially ambiguous, though, if one property has a name that is a prefix of another.

On the other hand, there's already a fairly simple workaround:

  public abstract static class Builder {
    public abstract Builder setName(String value);
    public abstract Builder setNumberOfLegs(int value);
    public ImmutableSet.Builder<String> countries() {
      return countriesBuilder();
    }
    abstract ImmutableSet.Builder<String> countriesBuilder();  // not public
    public abstract Animal build();
  }

@eamonnmcmanus eamonnmcmanus added Component: value P3 type=enhancement Make an existing feature better labels Jan 17, 2023
@perceptron8
Copy link
Contributor Author

I don't think using the method's return type alone would work in general.

You are right! And thank you for a workaround. However I see one more problem related to name conflicts that didn't came to my mind before. This already doesn't compile:

@AutoValue
public abstract class Animal {
	public abstract ImmutableSet<String> countries();
	public abstract ImmutableSet<String> countriesBuilder();
	
	public static Builder builder() {
		return new AutoValue_Animal.Builder();
	}
	
	@AutoValue.Builder
	public abstract static class Builder {
		public abstract ImmutableSet.Builder<String> countriesBuilder(); // error: [AutoValueBuilderReturnType] Method matches a property of sandbox.Animal...
		public abstract ImmutableSet.Builder<String> countriesBuilderBuilder();
		public abstract Animal build();
	}
}

Very unlikely to happen, sure. But possible.

As for workaround (thanks again!), it obviously forces builder to be an abstract class. With @AutoBuilder and no need for custom logic, it could be an interface. I can't imagine mocking a builder with java.lang.reflect.Proxy, but who knows, what crazy use cases live in the wild...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: value P3 type=enhancement Make an existing feature better
Projects
None yet
Development

No branches or pull requests

2 participants