-
Notifications
You must be signed in to change notification settings - Fork 283
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
CP-49677 CP-49647 Use URI for improved IPv6 in http.ml, newcli.ml #5676
CP-49677 CP-49647 Use URI for improved IPv6 in http.ml, newcli.ml #5676
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ Additional details and impacted files@@ Coverage Diff @@
## master #5676 +/- ##
========================================
- Coverage 51.4% 44.8% -6.6%
========================================
Files 13 16 +3
Lines 1928 2210 +282
========================================
+ Hits 991 992 +1
- Misses 937 1218 +281 see 4 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
) | ||
debug "%s: %s" __FUNCTION__ url ; | ||
let fail fmt = Printf.ksprintf failwith fmt in | ||
let query = function k, v :: _ -> (k, v) | k, [] -> (k, "") in |
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.
The previous implementation keeps all values for a key, is it significant, though?
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.
The URI module can represent a key with multiple values but our representation would have to use duplicate keys - which I think is very questionable and something we should avoid. Currently tests are passing, suggesting we don't use multiple values/keys.
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
55d0ab5
to
658a84b
Compare
in | ||
let reconstruct_uri uri = "/" ^ String.concat "/" uri in | ||
let data_of_uri uri = | ||
let uri, params = parse_uri (reconstruct_uri uri) in |
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.
Is parse_uri
(defined above) still used now? If not, it can be removed.
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.
still used; should be removed
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.
CP-49953 is a follow-up to remove it
The struture of Http.Url is unfortunate and we currently can't cange it but we can use a better and more concise implementation using URI. Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
658a84b
to
cb0ed6b
Compare
in | ||
Uri.( | ||
make | ||
let to_string scheme = |
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.
scheme
here isn't of the same type as in of_string
above, which is a little confusing. The type here is type t = scheme * data
.
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.
We should change the base type of Url, in the meantime this adapter is beneficial for parsing urls
We want to get away from string-based URL handling and delegate this to URI and get correct IPv6 handling along the way.