-
Notifications
You must be signed in to change notification settings - Fork 237
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 new CURLINFO_
constants and fn response_http_version()
#539
base: main
Are you sure you want to change the base?
Conversation
src/easy/handler.rs
Outdated
curl_sys::CURL_HTTP_VERSION_1_0 => HttpVersion::V10, | ||
curl_sys::CURL_HTTP_VERSION_1_1 => HttpVersion::V11, | ||
curl_sys::CURL_HTTP_VERSION_2_0 => HttpVersion::V2, | ||
curl_sys::CURL_HTTP_VERSION_3 => HttpVersion::V3, | ||
_ => unreachable!("TODO"), |
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.
According to https://curl.se/libcurl/c/CURLINFO_HTTP_VERSION.html this only returns a select few codes, but should we map them all "just in case"? It can also return 0
in case the version cannot be determined, should we map that "none" to Any
or wrap this in an Option
? And should we wrap other faulty codes into a Result
of sorts or panic on invalid library return values?
Sorry, still a bit new to this crate and curl
, while adding this change on a tangent of a tangent :)
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 assume that curl could return anything here, and the last thing we want to do is panic when encountering an unknown result. Curl is often dynamically linked, meaning we don't have control over which version of libcurl is actually used. If old Rust code using old curl-rust
code starts to run against a new libcurl version due to a system upgrade that starts returning a hypothetical "HTTP 4", then we:
- For sure should not panic on any return value
- If we can, return the underlying
i32
even though we don't understand it, allowing the caller to attempt to understand it themselves if they want to
One possible way of doing this would be to wrap the return value in a new enum that maybe looks something like this:
pub enum HttpVersionInfo {
HttpVersion(HttpVersion),
Unknown(i32),
}
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.
That conflicts a bit with the use of a Result
type. I'd rather wrap the existing Result<HttpVersion, CurlError>
with a enum HttpVersionSpecificError { CurlError(CurlError), UnknownValue(i32) }
of sorts :)
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 ended up using your suggestion as Unknown
isn't immediately invalid.
In another crate we've had discussions about a user hypothetically matching Unknown(0x1337)
, and the crate being bumped with a semver-compatible release (or breaking release, doesn't really matter here) that maps the new SuperDuperHttpVersion = 0x1337
value in this #[non_exhaustive]
enum
. The user will suddenly run into their mandatory _ => {}
catch-all arm again. Not giving them access to the value entices contributions and requests upstream to match the new value, and have less ambiguity inbetween.
src/easy/handler.rs
Outdated
/// | ||
/// Corresponds to `CURLINFO_HTTP_VERSION` and may return an error if the | ||
/// option isn't supported. | ||
pub fn response_http_version(&self) -> Result<HttpVersion, Error> { |
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.
This function ideally would be called http_version
to match the curl constant, but that's already taken. Perhaps instead should be called get_http_version
. We want the names to match the underlying curl names as much as possible, even if it is less understandable.
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.
Very understandable, but that name is already taken for the setter unfortunately.
EDIT: Great ninja-edit 😬
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.
Note that get_
prefixes are discouraged in Rust, but let's just use it here.
4eef5a5
to
c68c933
Compare
#[derive(Debug, Clone, Copy)] | ||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
pub enum HttpVersion { |
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.
Curious why simple enums have to be compared by match
/if let
and are not allowed to be used in simple ==
by deriving these quite-standard Eq
traits. Intentional or oversight?
Use `CURLINFO_HTTP_VERSION` (a new constant for the `curl-sys` crate) to determine the HTTP version used in the last/latest connection.
c68c933
to
13a600d
Compare
Looks like the CI isn't too happy about the new constants added to |
Fixes #538
Use
CURLINFO_HTTP_VERSION
(a new constant for thecurl-sys
crate) to determine the HTTP version used in the last/latest connection.