Skip to content
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

Add a way to put additional headers to handshake for connecting/reconnecting, see #865 #868

Merged
merged 2 commits into from Apr 8, 2019
Merged

Conversation

haruntuncay
Copy link
Collaborator

Description

Added a way to add headers in the handshake before connecting/reconnecting. Also added a method to delete a header, and to clear all headers.

Related Issue

Related to issue #865.

Motivation and Context

New Feature

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Collaborator

@marci4 marci4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be better to always use a TreeMap since it is also possible for the user to provide the HTTP-Header by the constructor call?

@marci4 marci4 added this to the Release 1.4.1 milestone Apr 7, 2019
* If the connection is already made, adding headers has no effect,
* unless reconnect is called, which then a new handshake is sent.<br>
* If a header with the same key already exists, it is overridden.
*/
Copy link
Collaborator

@marci4 marci4 Apr 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add:

  • @since 1.4.1
  • some description what key and value represent

here and for the other 2 new methods

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review. Of course, I am on it right now 😃

@haruntuncay
Copy link
Collaborator Author

Would it not be better to always use a TreeMap since it is also possible for the user to provide the HTTP-Header by the constructor call?

I am sorry, I couldn't understand what you meant. Could you please explain again ?

@marci4
Copy link
Collaborator

marci4 commented Apr 7, 2019

Would it not be better to always use a TreeMap since it is also possible for the user to provide the HTTP-Header by the constructor call?

I am sorry, I couldn't understand what you meant. Could you please explain again ?

My bad, sorry for that.
There is a constructor for the WebSocketClient, which does take a Map<String, String> as an argument (public WebSocketClient( URI serverUri, Map<String,String> httpHeaders).)

My question was if we should not simple add all these entries provided with the constructor argument into the TreeMap instead of using the provided one.

@haruntuncay
Copy link
Collaborator Author

Would it not be better to always use a TreeMap since it is also possible for the user to provide the HTTP-Header by the constructor call?

I am sorry, I couldn't understand what you meant. Could you please explain again ?

My bad, sorry for that.
There is a constructor for the WebSocketClient, which does take a Map<String, String> as an argument (public WebSocketClient( URI serverUri, Map<String,String> httpHeaders).)

My question was if we should not simple add all these entries provided with the constructor argument into the TreeMap instead of using the provided one.

So you are saying that we should instantiate private Map<String,String> headers; as a TreeMap and add each header entry from the constructor into that TreeMap, so we wouldn't need to re-instantiate and nullify each time addHeader clearHeaders are called, is that right ?

@marci4
Copy link
Collaborator

marci4 commented Apr 7, 2019

Would it not be better to always use a TreeMap since it is also possible for the user to provide the HTTP-Header by the constructor call?

I am sorry, I couldn't understand what you meant. Could you please explain again ?

My bad, sorry for that.
There is a constructor for the WebSocketClient, which does take a Map<String, String> as an argument (public WebSocketClient( URI serverUri, Map<String,String> httpHeaders).)
My question was if we should not simple add all these entries provided with the constructor argument into the TreeMap instead of using the provided one.

So you are saying that we should instantiate private Map<String,String> headers; as a TreeMap and add each header entry from the constructor into that TreeMap, so we wouldn't need to re-instantiate and nullify each time addHeader clearHeaders are called, is that right ?

I think nulling is fine, we just add each header entry from the constructor (if not null)

@haruntuncay
Copy link
Collaborator Author

Hi @marci4, Could you please check out the latest commit when you have the time :).
Thank you for the review.

Copy link
Collaborator

@marci4 marci4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you very much!

@marci4 marci4 merged commit 36a8102 into TooTallNate:master Apr 8, 2019
@haruntuncay haruntuncay deleted the issue#865 branch April 17, 2019 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants