-
Notifications
You must be signed in to change notification settings - Fork 4
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 blocking client #52
base: main
Are you sure you want to change the base?
Conversation
This adds the sync HTTP client [ureq](https://lib.rs/ureq) as well as [maybe-async](https://lib.rs/maybe-async) for conditional compilation. You can now use a blocking client by setting the `blocking` feature flag.
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'm liking the smaller binary size that the blocking version benefits from by using ureq
instead of reqwest
.
I'm not entirely sure about the maybe_async
approach, so Id like to try it out before updating the mainline client.
One thing that this diff could use is the following in Cargo.toml:
[[example]]
name = "cli"
[[example]]
name = "ingest-hn"
.map(|s| s.to_str()) | ||
.transpose() | ||
.map_err(|_e| Error::InvalidQueryId)? | ||
.get_header("X-Axiom-History-Query-Id") |
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.
Not a thing to be solved in this diff, but here's a Rustacean thought: How I'd like this to be a type that represents the id there.
Something like an
enum AxiomId {
Nano(str),
Fallback(uuid::Uuid)
}
impl From<AxiomId> for String {}
impl TryFrom<String> for AxiomId{}
I'm not a fan of passing strings around like it's Python 2 in 2010 :)
gzip_payload.finish() | ||
}; | ||
|
||
#[cfg(all(feature = "tokio", not(feature = "blocking")))] |
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.
Hmm. I'd done this before, and I always considered it a code smell.
From the docs :
There are rare cases where features may be mutually incompatible with one another. This should be avoided if at all possible, because it requires coordinating all uses of the package in the dependency graph to cooperate to avoid enabling them together.
I'd like to see how the maybe_async
approach works in practice to form a more informed opinion though. Initially, I think I'd find it weird that the function signatures changed under me by flipping a feature flag.
I'd also wonder about testing - typically tests make the most sense when run with --all-features, so this wonuld leave some paths untested.
Again, I'd like to see this in practice :)
use http::HeaderMap; | ||
use serde::de::DeserializeOwned; | ||
use std::time::Duration; | ||
use ureq::{Agent, Middleware, MiddlewareNext, Request}; |
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.
Nice 👍
This adds a blocking client which can be enabled using the
blocking
feature.It works exactly like the async client but doesn't expose async fns as well as stream ingest methods.