- Notifications
You must be signed in to change notification settings - Fork556
Add CharSequence overloads for putHeader methods in HttpRequest#2783
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
Vynbz commentedAug 31, 2025
While |
tsegismont left a comment
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.
@Vynbz thanks for the PR.
| */ | ||
| @Fluent | ||
| @GenIgnore(GenIgnore.PERMITTED_TYPE) | ||
| HttpRequest<T>putHeader(CharSequencename,CharSequencevalue); |
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.
Can you create a default implementation that converts toString?
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've updated the implementation to use interface default methods as you suggested
tsegismont commentedSep 10, 2025
Please go ahead and add the method. You can update |
tsegismont left a comment
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.
Thanks for the updates@Vynbz
In fact, I was suggesting to add default implementations, so that existing implementations in other projects don't break with the new release.
But in our project, we must implement these new methods in an efficient manner (always converting to string defeats the original purpose of this PR)
Vynbz commentedSep 12, 2025
@tsegismont thanks for review Just to make sure I understand correctly before I push the changes - you want me to keep the default implementations in the HttpRequest interface, but also keep efficient override implementations in HttpRequestImpl that pass the CharSequence directly to headers().set() without converting to String? Also regarding the tests, I've updated them to handle the ambiguous method calls by adding CharSequence cast - is this the right approach? |
tsegismont commentedSep 15, 2025
Exactly
Yes |
1373084 to2eda11eCompareVynbz commentedSep 15, 2025
@tsegismont Done! Changes implemented as discussed and commits squashed |
Motivation
As discussed in#2770, the
HttpServerResponseinterface in vertx-core already provides CharSequence overloads for header methods, but the web client'sHttpRequestinterface only accepts String parameters. This inconsistency means users need to convert CharSequence values (like Netty'sHttpHeaderNamesconstants) to Strings unnecessarily.