search

googleapis / sdk-platform-java

Tooling and shared libraries for Cloud SDK for Java
https://cloud.google.com/java/docs/bom
Apache License 2.0
64 stars 52 forks source link

Consider removing abstract toBuilder #1306

Open igorbernstein2 opened 6 years ago

igorbernstein2 commented 6 years ago

Gax uses a pattern of abstract <B extends Builder<SettingsT, B>> B toBuilder(); in a base class, expecting a subclass to override it with a concrete return type.

This causes warnings like:

BigtableTableAdminStubSettings.java:[764,43] build() in com.google.cloud.bigtable.admin.v2.stub.BigtableTableAdminStubSettings.Builder overrides <B>build() in com.google.api.gax.rpc.StubSettings.Builder
  return type requires unchecked conversion from com.google.cloud.bigtable.admin.v2.stub.BigtableTableAdminStubSettings to com.google.api.gax.rpc.StubSettings<B>

It might be cleaner remove the abstract toBuilder() method from the base classes and trust that the subclasses will implement that method

igorbernstein2 commented 6 years ago

@garrettjonesgoogle can you take a look at this?

garrettjonesgoogle commented 6 years ago

It seems like it should be possible to fix the parameterization to get this to work...

igorbernstein2 commented 6 years ago

I don't think this is possible. The base class's toBuilder return type would have to include a self referential parameter that includes its builder class, which will be an inner class that hasn't been defined yet.

elharo commented 4 years ago

This is likely an API breaking change and a major version update. Is it worth that?

igorbernstein2 commented 4 years ago

I don’t think it’s worth doing a major version bump for this, but I would argue that if a major version were to happen, this should be included. Maybe this can be included in a v2 hotlist? (Along with making ClientContext internal)

meredithslota commented 2 years ago

We missed the opportunity for v2 — is this still worth doing at the next breaking change?

meltsufin commented 2 years ago

I'm not sure when we'll have the next major version bump, but yes, it would have to be filed under that.