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

Merge Google changes as of 2016-05-10 #328

Merged
merged 17 commits into from
May 16, 2016

Conversation

eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented May 10, 2016

  • Allow an @AutoValue.Builder to extend a parent builder using the <B extends Builder<B>> idiom.
  • Add support for "optional getters", where a getter in an AutoValue Builder can have type Optional<T> and it will return Optional.of(x) where x is the value that has been set in the Builder, or Optional.empty() if no value has been set.
  • In AutoValue builders, support setting a property of type Optional<T> via a setter with an argument of type T.
  • In AutoValue builders, make optional properties default to Optional.empty().
  • Have AutoAnnotation factor in package names when detecting overloads. Previously it treated all annotations with the same SimpleName as being overload attempts.
  • Support for TYPE_USE @Nullable. This is patch for "AutoValue should respect @Nullable annotations on types #283" #293 by @brychcy.
  • Restructure the code in AutoValueProcessor for handling extensions to get rid of warnings about abstract methods when those methods are going to be implemented by an extension, and to fix a bug where extensions would not work right if there was a toBuilder() method. Some of the code in this change is based on Moves method verification after extensions have had the chance to consume properties. #299 by @rharter.
  • Remove an inaccurate javadoc reference, which referred to an artifact from an earlier draft version of the Extensions API. This is Removing inaccurate JavaDocs reference #322 by @lucastsa.
  • Add logic to AutoValue to detect the confusing case where you think you are using JavaBeans conventions (like getFoo()) but you aren't because at least one method isn't.
  • Add a README.md describing EscapeVelocity.
  • Add a bit more information in the changelog about 1.0 and 1.1. Also quote some instances of @AutoValue that needed it.

cgruber and others added 17 commits May 10, 2016 08:35
…uote some instances of `@AutoValue` that needed it.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=119101365
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=119453753
git commit: dfcf55e5fc3f29ac55847a19627f9f15d3aff370
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=119557171
…est practices: []

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=119568091
…ou are using JavaBeans conventions (like getFoo()) but you aren't because at least one method isn't. Then a Builder setter like setFoo will be rejected because it would have had to be called setGetFoo. This change detects that this might have happened and shows a list of the methods that prevented the JavaBeans conventions from being applied.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=119569372
… from an earlier draft version of the Extensions API.

This is google#322 by @lucastsa.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=119896497
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120482220
…his involves moving a fair amount of code around, but the resultant organization for method identification should be clearer. The principal purpose of the restructuring is to get rid of warnings about abstract methods when those methods are going to be implemented by an extension. A second purpose is to fix a bug where extensions would not work right if there was a toBuilder() method.

Also improve the test coverage in ExtensionTest, which was fairly minimal.

Some of the code in this change is based on google#299 by @rharter.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120485500
Limitations: The following cases currently don't work with javac (i.e.
the annotation processors don't see the @nullable so a null check is
generated in the constructor):

public abstract int @nullable [] getInts()
This works with eclipse JDT and the null check is ommited in the
generated code, but note that the @nullable is in the wrong position in
the generated code (i.e. @nullable int[] instead of int @nullable[])),
which might lead to compiler warnings

For generic type T:
@nullable
public abstract T getT()
(works in JDT)

Also @nullable won't work for types which can't be imported because of
naming conflicts, because the @nullable is generated at the wrong place,
i.e. "@nullable some.package.Map" instead of "some.package.@nullable
Map" which won't compile at all for TYPE_USE @nullable.

This change was imported from github: google#293
The original author is Till Brychy.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120629206
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120629767
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120714957
… Previously it treated all annotations with the same SimpleName as being overload attempts.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120715929
…mpty(). Also, in the AutoValue_Foo constructor, do not check for nullness if there is a builder, because the constructor is private and the build() method will check for nullness before calling it.

The effect on generated code is visible in CompilationTest (I recommend "Expand whole file").
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120933175
… via a setter with an argument of type T.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120943277
…ilder can have type Optional<T> and it will return Optional.of(x) where x is the value that has been set in the Builder, or Optional.empty() if no value has been set.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=121591500
…xtends Builder<B>> idiom.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=121886725
@cgruber
Copy link
Contributor

cgruber commented May 16, 2016

LGTM

@eamonnmcmanus eamonnmcmanus merged commit ed224a8 into google:master May 16, 2016
@eamonnmcmanus eamonnmcmanus deleted the merge-2016-05-10 branch May 16, 2016 19:47
@lucastsa
Copy link

"In AutoValue builders, support setting a property of type Optional via a setter with an argument of type T."

Why did you use Optional.of() instead of Optional.ofNullable() in that implementation? It makes it impossible to reset that property after you set it in that way (specially after getting a builder from toBuilder()), unless you have a setter with an Optional argument.

@eamonnmcmanus
Copy link
Member Author

This probably isn't the best place to have this discussion, but we did deliberately decide against allowing a null argument here. We didn't feel that it would be appropriate to distort the semantics just because null and empty feel sort of like the same thing. It's at least as likely that you would want to get a NullPointerException when the argument is null as that you would want Optional.empty(). This is any case only a minor convenience. You can always write explicitly:

@AutoValue.Builder
public abstract static class Builder {
  public Builder setFoo(String foo) {
    return setFoo(Optional.ofNullable(foo));
  }
  abstract Builder setFoo(Optional<String> foo);
}

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

Successfully merging this pull request may close these issues.

6 participants