-
Notifications
You must be signed in to change notification settings - Fork 268
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
Replace http-types with http #2233
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution @ntrinquier! We will review the pull request and get back to you soon. |
@microsoft-github-policy-service agree company="Helsing" |
This does get rid of a lot of older dependencies as well, since it seemed to pull in strict dependencies (probably using the older resolver; I never looked exactly). Might be worth it on that alone. I'm still pondering on whether it's even worth it. Do we really need compatible types with other apps? Our generated clients are passing the right HTTP method. We do expose the StatusCode but could just as easily return our own. @ntrinquier is there any benefit to returning a common @analogrelay @LarryOsterman what do you think? To summarize, I'm leaning toward defining our own HTTP method enum (customers should never need to pass this anyway; it's public so our other crates can use it from |
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.
Thank you for your contribution! I'm leaning toward taking this, in part, but reconsidering whether we even should expose third-party StatusCode
from our error and response types. After GA, we have pretty strict no-breaking changes policy, though it's good to see that http
did declare a 1.x stable release so, as long as they stick to semver, we should be stable.
Still, the only reason we expose Method
from azure_core
is because other crates need it to call into client methods. It's not meant to be truly customer public. StatusCode
obviously is, but we could just as easily return our own with, optionally, From
impls to http::StatusCode
...perhaps behind a feature flag so the dependency is optional (if not already transitive) by default.
We'll talk internally about our public API shape, but I'd love to hear if you can think of any good reason to expose Method
other than just having to since we have separate crates.
@@ -83,7 +83,7 @@ fe2o3-amqp-types = { version = "0.12" } | |||
futures = "0.3" | |||
getrandom = { version = "0.2", features = ["js"] } | |||
hmac = { version = "0.12" } | |||
http-types = { version = "2.12", default-features = false } | |||
http = { version = "1.2", default-features = false } |
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.
Based on all the build errors, we need at least std
.
Not really, I went with the straightforward implementation which is a drop-in replacement, and so it preserves the status quo. I do not see a problem with defining our own types, and in fact I think it would be nice to hide the dependency behind a feature flag by default, because new major versions of ubiquitous dependencies like Could you let me know after you get the chance to discuss internally? Then I can finalize the PR based on your feedback and guidance! |
True; however, I did check and we already have transitive dependencies on |
As mentioned in #1937,
http-types
is not being maintained (and neither are some its dependencies).This replaces it with the
http
crate which seems to be the ecosystem standard (10x more recent downloads).