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 Duration and Supplier<Duration> overloads for Supplier#memoizeWithExpiration #3691

Conversation

mfboulos
Copy link
Contributor

@mfboulos mfboulos commented Nov 8, 2019

Directly addresses #3688

I don't want to step on any toes since I saw that long, TimeUnit overloads are in the cards for several classes, but I also see the use in a Duration supplier, so I went and wrote this up real quick so we can at least have it in the works.

One thing I noticed while writing this is that com.google.common.util.concurrent.Internal#toNanosSaturated is package-scoped internal, but the only method in there is applicable to all legacy API's that require a long, TimeUnit, and there are several scattered in base, cache, and collect under com.google.common. It might be worth considering gauging the need for it to public-scoped, if at some point the human race figures out how to prolong life indefinitely.

Also first contribution here, hope all looks good :)

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@mfboulos
Copy link
Contributor Author

mfboulos commented Nov 8, 2019

@googlebot I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Nov 8, 2019
@jbduncan
Copy link
Contributor

jbduncan commented Nov 8, 2019

Hi @mfboulos, welcome to Guava. :)

I think it's worth mentioning that it's unlikely this PR will be merged, because the Guava team prefer to do API reviews internally (which is apparently the hard part) and then write the code themselves to match whatever API they've agreed on.

See the contribution guidelines for more information.

@jbduncan
Copy link
Contributor

jbduncan commented Nov 8, 2019

That being said, I'm sure that this code would be a great start for @chriskilding's needs, who opened #3688, if you're happy for them to take the code as-is. :)

@kluever
Copy link
Member

kluever commented Nov 8, 2019

It might be worth considering gauging the need for it to public-scoped

FWIW, we have a copy of this in our (still internal) com.google.common.time package, which we hope to get around to releasing at some point.

@mfboulos
Copy link
Contributor Author

mfboulos commented Nov 8, 2019

Understandable! My intuition was to put this PR in there just to have the code handy for you guys, but I understand there's more discussion behind it since it's essentially an extension to the existing API.

copybara-service bot pushed a commit that referenced this pull request 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 pull request 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 pull request 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: 597280125
@cpovirk
Copy link
Member

cpovirk commented Jun 21, 2024

We added the Duration overload recently, including under Android. I don't know that we'll end up adding a Supplier<Duration> overload at all, but we can come back to this PR if we someday resume the discussion on #3688 and decide to go for it.

@cpovirk cpovirk closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants