-
Notifications
You must be signed in to change notification settings - Fork 22
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
Deprecate Guava Multimap and Function in favor of JDK & Remove Guava collections #217
Conversation
e7a307e
to
92a3dc3
Compare
I've searched our internal repo and found a lot of setHeader(Multimap) usage. So 2 looks better. |
40d7eef
to
56ae642
Compare
74ca255
to
b31fe3a
Compare
b31fe3a
to
a10823c
Compare
Thanks. I'll take a look |
return builder.build(); | ||
} | ||
|
||
public Map<String, Collection<String>> getHeaderParamsV2() |
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 will be the default method after deprecating getHeaderParams(). How about using a more regular method name like getAllHeaders()
but a different name from getHeaderParams()?
ok. Basically LGTM. @exoego Only one suggestion to use a normal method name without any version suffix https://github.com/treasure-data/td-client-java/pull/217/files#r1119311242 |
Part of #135
This is the final Guava deprecation and removal.
After this PR and a new minor version cut, I think we could delete the deprecated one and guava dependency.
The problem is
Multimap<K, V>
. How should we remove it?Possible directions:
Map<K, Collection<V>>
and introduce breaking change aggressively.Map<K, Col<V>>
and deprecate the old methods withMuiltimap
.TDClientConfig#headers
andTDApiRequest#getHeaderParams()
. See new members~V2
.I propose picking 2nd since it helps users to migrate.
WDYT?