-
Notifications
You must be signed in to change notification settings - Fork 41
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
Allow hostname verifier and socket factory for http component #29
Conversation
mmoayyed
commented
Sep 24, 2021
•
edited
Loading
edited
- Allows one to specify a hostname verifier and socket factory when creating instances of Http.
- Overloads the constructor to keep backward compatibility.
*/ | ||
public Http(String inMethod, String inHost, String inUri, int timeout) { | ||
public Http(String inMethod, String inHost, String inUri, int timeout, |
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 wonder if we should move to a Builder at this point? It's probably overkill for this change, but it would set us up in a better position going forward.
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 that would be a good direction for future config changes.
@mmoayyed I have introduced a Builder for the Http class as discussed above |
Cool. Would you also cut a new release for the change? Thank you. |
@mmoayyed Sorry, your message got lost in the shuffle. I'll get this change released ASAP. |
I started the release process. It should be visible on Maven in a couple hours, and I'll update the README when I confirm that. |
Thank you Aaron. |
@AaronAtDuo Reviewing the latest release, I don't see how one could set the hostname verifier and socket factory. Are these options are not taken into account with the new builder component, which was really the original intention of this pull request? I am not sure what the advantage of the builder at the moment is, as it seems to not support things I actually wanted to modify in this PR , right? Or am I missing something? |
@mmoayyed I apologize, I didn't mean to imply earlier that the functionality you wanted had yet been implemented. I introduced the Builder pattern without adding new functionality, and then I intended to come back and add the hostname verifier and socket factory piece in a follow up commit. But I have not yet had a chance to do the second change. |
#35 added to track this |
Got it. Thanks very much! |