google / auto

A collection of source code generators for Java.
Apache License 2.0
10.43k stars 1.2k forks source link

Immutable*Collection*.Builder methods can't begin with 'get' #387

Open Wolvereness opened 8 years ago

Wolvereness commented 8 years ago
@AutoValue
abstract class MyClass {
    abstract ImmutableList<String> getBlobs();
    Builder builder() {
        return new AutoValue_MyClass.Builder();
    }
    @AutoValue.Builder
    abstract static class Builder {
        abstract ImmutableList.Builder<String> getBlobsBuilder();
        abstract MyClass build();
    }
}

Results in:

Error: ... : Method without arguments should be a build method returning MyClass or a getter method with the same name and type as a getter method of MyClass

Currently, you must name getBlobsBuilder() as blobsBuilder() to function. As every other builder method supports bean style setters, the collections builders should support bean style getters.

eamonnmcmanus commented 8 years ago

One issue is that you could conceivably have properties in MyClass called getBlobs() and getBlobsBuilder() and in that case getBlobsBuilder() in the builder class would be a getter for the getBlobsBuilder() property rather than a builder for the getBlobs() property. I'd also say that blobsBuilder() isn't really a property in the Bean sense: usually a property with getter getFoo() is modified with a setter setFoo(x), whereas the purpose of blobsBuilder() is to be mutable and allow you affect the value of a different property.

Having said that, there's nothing preventing you defining an alias for the generated blobsBuilder() method, like this:

@AutoValue
public abstract class MyClass {
    public abstract ImmutableList<String> getBlobs();
    public Builder builder() {
        return new AutoValue_MyClass.Builder();
    }
    @AutoValue.Builder
    public abstract static class Builder {
        public ImmutableList.Builder<String> getBlobsBuilder() {
            return blobsBuilder();
        }
        abstract ImmutableList.Builder<String> blobsBuilder();
        public abstract MyClass build();
    }
}
Wolvereness commented 8 years ago

One issue is that you could conceivably have properties in MyClass called getBlobs() and getBlobsBuilder() and in that case getBlobsBuilder() in the builder class would be a getter for the getBlobsBuilder() property rather than a builder for the getBlobs() property.

I'm not following; at what point do builders generate getters for properties? Validation heavily implies you need to do full construction to examine the values, but if getters in the builder is a feature it should be mentioned in the documentation.

I can understand that this proposal creates a strange situation where you have a setBlobsBuilder() method in the builder that has nothing to do with getBlobsBuilder(), but that kind of ambiguity is a problem independent of this proposed change. blobs() needing a builder with blobsBuilder() as a property is already possible, and it would be reasonable to either fail fast or let the parameter (on the setter) distinguish the name overload.

I'd also say that blobsBuilder() isn't really a property in the Bean sense: usually a property with getter getFoo() is modified with a setter setFoo(x), whereas the purpose of blobsBuilder() is to be mutable and allow you affect the value of a different property.

Equating builders to beans doesn't seem appropriate. However, I assert that the bean naming convention is already active regardless of how bean-like builders are, and this is an exception that should be addressed. And, it behaves more like a bean (returning the same value, even if mutable) than other conventions (returning a new builder).

Having said that, there's nothing preventing you defining an alias for the generated blobsBuilder()

I do like that idea for my own use if I would like better support for languages that treat bean-like methods as fields (like groovy), but my particular motivation for this request was the time I spent figuring out why I had such a cryptic error message when I expected naming consistency. A less cryptic message might be just as complicated a proposal as allowing it to named as a getter, but specific documentation of this difference would have also helped.

eamonnmcmanus commented 8 years ago

Getters are described in the section immediately following the one you cited.

I think there are two things we can do to address the issue here:

  1. Say more explicitly in the documentation that even if you are using getFoo() names for your properties, the builder method is still called fooBuilder().
  2. Perhaps improve the error code so that it detects problems like this.

I still don't think that the builder method should be called getFooBuilder().