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 HTTP util methods with custom HTTP headers #218

Merged
merged 2 commits into from
Dec 5, 2016

Conversation

chrismayer
Copy link
Collaborator

@chrismayer chrismayer commented Nov 14, 2016

This adds methods to the HttpUtil class offering the possibility to add custom HTTP headers to the requests.

Would be great if the original devs of the HttpUtil class can have a look at this and if they agree in general I would finalize this by adding some tests and API docs.

@marcjansen
Copy link
Member

marcjansen commented Nov 15, 2016

I am not opposed to this and would be plus 1 for merging (after comments and tests are added).

With this in, we will have > 90 static public methods in this utility class. Basically all of them are overloaded. Looking through the diff was already quite hard. Is there anything we can do to have this less verbose?

@buehner, do you want to add something?

@buehner
Copy link
Member

buehner commented Nov 16, 2016

I am not one of the original devs of this class (--> @ahennr and @dnlkoch) but i checked out the http-util-headers branch and could successfully use it.
As the changes seem to be backwards-compatible (through overloading!?), i dont have a problem to merge this. If there are tests missing, it would be nice to add them.

@ahennr
Copy link
Member

ahennr commented Nov 24, 2016

👍 from me as well. As @buehner mentioned above, the addition of missing tests would be fine.

This adds methods to the 'HttpUtil' class offering the possibility to add
custom HTTP headers to the requests.
@chrismayer
Copy link
Collaborator Author

Thank you all for your feedback. I just finalized this and added tests.

@buehner
Copy link
Member

buehner commented Dec 5, 2016

I just checked out your changes and tested them with different existing projects to assure backwards compatability.

As everything is fine, i'll merge now!

Thx for the work @chrismayer

@buehner buehner merged commit b6d0ebd into terrestris:master Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants