-
Notifications
You must be signed in to change notification settings - Fork 33
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
Make futures-extra compatible with Guava 20. #27
Conversation
@@ -2,7 +2,7 @@ | |||
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> | |||
<modelVersion>4.0.0</modelVersion> | |||
<groupId>com.spotify</groupId> | |||
<version>2.6.3-SNAPSHOT</version> | |||
<version>3.0.0-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the version bump? There are no breaking changes as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Futures.transformAsync
appeared only in Guava 19, stated compatibility of current version is Guava 18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought I had to do some kind of version bump to signal that Guava requirements had changed. But maybe 3.0 was over the top. Bumped to 2.7 now instead. Ok?
pom.xml
Outdated
@@ -2,7 +2,7 @@ | |||
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> | |||
<modelVersion>4.0.0</modelVersion> | |||
<groupId>com.spotify</groupId> | |||
<version>2.6.3-SNAPSHOT</version> | |||
<version>2.7.0-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No signatures changed, so this should still be 2.6.3
... However I'll still merge this if you don't want to fix this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the need for Guava major bump should be reflected somehow in our version numbering. In addition I deprecated a public method.
@@ -71,6 +71,9 @@ This is just a simple delegating method that explicitly calls | |||
Futures.transform(future, Function). There is also a corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line doesn't seem to be correct any longer, IIUC? The call is instead to Futures.transformAsync
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that is correct. It is Futures.transform(future, Function)
but Futures.transformAsync(future, AsyncFunction)
. They used to have Futures.transform
overloaded with both Function and AsyncFunction arguments. In Guava 19 they added transformAsync and deprecated the transform variant that took AsyncFunction in preparation for removing it in Guava 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
No description provided.