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

Add java.time.Duration overloads to Suppliers.memoizeWithExpiration() #3169

Closed
cowwoc opened this issue May 30, 2018 · 7 comments · Fixed by #6909
Closed

Add java.time.Duration overloads to Suppliers.memoizeWithExpiration() #3169

cowwoc opened this issue May 30, 2018 · 7 comments · Fixed by #6909

Comments

@cowwoc
Copy link

cowwoc commented May 30, 2018

Suppliers.memoizeWithExpiration() takes a TimeUnit. Consider adding a java.time.Duration overload similar to what you did with CacheBuilder.

@kluever
Copy link
Member

kluever commented May 30, 2018

Funny you bring that up...I just had a need for that last week and have an internal CL out that adds the java.time.Duration overload. However, the CL has been stalled because we're trying to determine what to do for the parameter and return types re. legacy, soft-deprecated com.google.common.base.Supplier vs. Java's java.util.function.Supplier.

Should we accept the j.u.f.Supplier but return the c.g.c.b.Supplier? (i.e., "liberal in what you accept, conservative in what you return" - Robustness Principle)?

Should we accept and return j.u.f.Supplier?

Other ideas?

@cowwoc
Copy link
Author

cowwoc commented May 30, 2018

@kluever
My 2 cents: all Guava APIs that expect a c.g.c.b.Supplier argument should allow j.u.f.Supplier as well. Where they don't already do so, overloads should be added. I would even go so far as formally deprecating c.g.c.b.Supplier explicitly.

Once this is done, I see no benefit in returning com.google.common.base.Supplier.

@kluever
Copy link
Member

kluever commented May 30, 2018

All APIs that accept a c.g.c.b.Supplier already can accept a j.u.f.Supplier, because c.g.c.b.Supplier extends j.u.f.Supplier.

For this particular case:
The argument for accepting j.u.f is that now folks can pass either j.u.f or c.g.c.b to the method. Of course both of these would work if we accepted c.g.c.b instead.
The argument for returning c.g.c.b is that folks can assign the return type to either a variable of type j.u.f or c.g.c.b.

@cowwoc
Copy link
Author

cowwoc commented May 30, 2018

@kluever I would suggest hard-deprecating c.g.c.b.Supplier to simplify these kind of decisions. That being said, here is my reasoning:

  • Users want to use a new Java 8 API: Duration
  • This means their codebase has access to j.u.f.Supplier
  • If we use j.u.f.Supplier exclusively, we're telling them if you want to use this Java8-specific API then you also need to update your code to use this other Java8-specific API.

After all, users don't need to "upgrade" to using Duration in the first place. If they're already making the migration from TimeUnit this is just a related upgrade for them to make.

@kluever
Copy link
Member

kluever commented May 30, 2018

I tend to agree with you. It is a little weird that we're mixing APIs that accept/return c.g.c.b.Supplier and j.u.f.Supplier in a class called c.g.c.b.Suppliers, but I'm not sure where else they would go.

@nikhilbarar
Copy link
Contributor

Can i pick this up?

Will this work:

public static <T> c.g.c.b.Supplier<T> memoizeWithExpiration(
      c.g.c.b.Supplier<T> delegate, j.t.Duration duration)

@kluever
Copy link
Member

kluever commented Jun 4, 2018

@nikhilbarar thanks for the offer, but I actually have a pending change out for this - we just need to work through the Guava Supplier vs. JDK Supplier issue.

@raghsriniv raghsriniv added the P3 label Jun 24, 2019
@cpovirk cpovirk added P2 and removed P3 labels Jan 9, 2024
@cpovirk cpovirk assigned cpovirk and unassigned kluever Jan 10, 2024
copybara-service bot pushed a commit that referenced this issue Jan 10, 2024
This CL contains part but not all of #3691

Fixes #3169

We'd considered this previously in cl/197933201 but gotten stuck on the question of whether to accept and/or return the `java.util.function` type. My claim is that we want an overload that is near-identical to the old one for ease of migration. Additionally, if we want to support `java.util.function.Supplier` in the future, then we're going to have to add an overload of the existing `memoize` method, and which point it would probably look _more_ weird if we had 2 overloads for `memoize` (and maybe `synchronizedSupplier`) but only a single `memoizeWithExpiration` method.

RELNOTES=`base`: Added a `Duration` overload for `Suppliers.memoizeWithExpiration`.
PiperOrigin-RevId: 597004403
copybara-service bot pushed a commit that referenced this issue Jan 10, 2024
This CL contains part but not all of #3691

Fixes #3169

We'd considered this previously in cl/197933201 but gotten stuck on the question of whether to accept and/or return the `java.util.function` type. My claim is that we want an overload that is near-identical to the old one for ease of migration. Additionally, if we want to support `java.util.function.Supplier` in the future, then we're going to have to add an overload of the existing `memoize` method, and which point it would probably look _more_ weird if we had 2 overloads for `memoize` (and maybe `synchronizedSupplier`) but only a single `memoizeWithExpiration` method.

RELNOTES=`base`: Added a `Duration` overload for `Suppliers.memoizeWithExpiration`.
PiperOrigin-RevId: 597004403
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.

5 participants