Http client add url support #833
Conversation
|
@ohamuy thanks for your contribution. Could you please fix the format? |
Codecov Report
@@ Coverage Diff @@
## main #833 +/- ##
==========================================
- Coverage 95.50% 95.48% -0.01%
==========================================
Files 156 156
Lines 6614 6614
==========================================
- Hits 6316 6315 -1
- Misses 298 299 +1
|
|
@ThomsonTan Done! Also should I add to the change log for this commit? |
| @@ -227,8 +227,11 @@ class HttpClient | |||
| { | |||
| public: | |||
| virtual std::shared_ptr<Session> CreateSession(nostd::string_view host, | |||
| uint16_t port = 80) noexcept = 0; | |||
| virtual bool CancelAllSessions() noexcept = 0; | |||
| uint16_t port) noexcept = 0; | |||
There was a problem hiding this comment.
Do we still need the port parameter here, if the URL itself can contain scheme and port?
Should we rename the host parameter to url or uri ?
| virtual bool CancelAllSessions() noexcept = 0; | ||
| uint16_t port) noexcept = 0; | ||
|
|
||
| virtual std::shared_ptr<Session> CreateSession(nostd::string_view url) noexcept = 0; |
There was a problem hiding this comment.
Okay, I see what you are doing. But I am not sure - maybe we should just deprecate the old method? I think this new one is much better. If we look at Javascript API for the open method, it only requires the URL parameter. In Java - this is also completely outsourced to java.net.URL . It's even more logical to assume that the session itself accepts URL object or string, rather than trying to implement multi-parameter parsing from host, port. Because it's rather [ scheme, host, port, location ] than simply [ host, port ] pair. It'd be best we just collapse all of it into single string parameter, then parse it using URL class.
There was a problem hiding this comment.
Yes exactly, I just overloaded the function because that is what the @lalitb (the original issue creator) said might have been good. If its alright I can just simply remove the old parameter and just leave the new one since its also consistent with the GET and POST commands. But that is definitely a decision for the maintainers so when they give their input we can see whats the best course of action. @lalitb @reyang
There was a problem hiding this comment.
Yes, it makes sense to remove the original method as @maxgolov suggested.
…method, simplified original Session constructor
|
@lalitb @maxgolov So I implemented the changes. Final change list: -Deleted old |
|
Ok so apparently removing the original function broke a lot of tests, what should I do? |
Doesn't look like a lot - 3 files with tests... Fix them? 😃
Almost there! |
|
Thank you for your contribution. Looks good to merge now :) |
HTTPClient : Support for full URL argument
Fixes # 812
Overloaded the CreateSession function allowing for url input:
Added it in the header file http_client.h
Implementation in http_client_curl.h
The function allows for url input instead of just host & port, utilizing the url parser in common folder. It returns an empty/non-existent host, and default port(80) if the parsing fails, and continues similarly to the original function otherwise.
One Unit test was changed to test the overloading function, but the overall functionality is not changed as the other original function parameters are tested in other unit tests. All tests passed, file builds successfully.
Lastly, the default port of 80 was removed from the original function because if the user ends up not providing a port, it will go through the other overloaded function and the url parser gives it a default port of 80
@lalitb @reyang