-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add some useful API methods #1295
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
Conversation
... and set headers method for headers with single values
Hi,
There's too many different things in this PR. It would be better to isolate them.
I'm OK with the first one (once again, I'm so sad/disappointed by Java's type erasure...). See comments.
Regarding the second one, I'm not fond of it depending on the Servlet API.
It would introduce a required additional dependency to the Servlet API for very limited value.
I'm fine with the io.netty.handler.codec.http.cookie.Cookie bridge though, as it ships with Netty. There's also a chance that I'll drop AHC class for the Netty one.
I would be OK with isolated bridges that only require an optional dependency, though.
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.
Please revert imports ordering: unrelated.
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.
please revert
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.
missing null check, like in setHeaders
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.
add a clearHeaders method?
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.
missing Javadoc
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.
Are you sure this dep is really optional? I feel like the Cookie class would fail to load and crash.
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, just import doesn't mean anything. Cookie class would be loaded (and fail) if somebody actually would use these new methods.
@slandelle +1 for dropping cookie in favour of Netty's Cookie.
I would be OK with isolated bridges that only require an optional dependency, though.
Current code should be absolutely fine with optional dependency (so, if nobody will add javax.servlet and would not use new conversion methods - there would be no ClassDefNotFound), but I would double check.
Closing this.
Let's aim at dropping AHC's own Cookie impl for AHC 2.1. I just want to make sure I contributed everything upstream and we don't lose anything in the move.
Motivation and proposals:
setHeaders(Map<String, Collection<String>> headers)is general, but in most cases applications use headers with only one value, so it's good to have a simple method to set them without convertingMap<String, String>toMap<String, Collections.singletonList<String>>.org.asynchttpclient.Cookiemanually. Expose two helper factory methods set for Netty's and standardjavax.servletentities.These methods were implemented as static utils for async-http-client in my project, I think it would be handy to have them out of the box.