Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: make HTTP provider optional #379

Merged
merged 8 commits into from
Mar 29, 2024
Merged

feat: make HTTP provider optional #379

merged 8 commits into from
Mar 29, 2024

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Mar 23, 2024

Closes #199

mattsse
mattsse requested changes Mar 24, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pedantic nit

otherwise sounds reasonable to me

crates/provider/src/lib.rs Outdated Show resolved Hide resolved
mattsse
mattsse approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, pending @DaniPopes

onbjerg
onbjerg approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm too

@prestwich
Copy link
Member

@leruaa this has conflicts with #318 which was just merged. I can clean them up if you don't feel like you understand the code. let me know

@leruaa
Copy link
Contributor Author

leruaa commented Mar 26, 2024

I'll give a try

@leruaa
Copy link
Contributor Author

leruaa commented Mar 26, 2024

@prestwich I think the merge went fine

prestwich
prestwich requested changes Mar 28, 2024
View reviewed changes
crates/provider/Cargo.toml Outdated Show resolved Hide resolved
crates/provider/Cargo.toml Outdated Show resolved Hide resolved
prestwich
prestwich reviewed Mar 29, 2024
View reviewed changes
crates/provider/src/builder.rs Outdated
@@ -225,23 +226,25 @@ impl<L, N> ProviderBuilder<L, N> {
Ok(self.on_client(client))
}

/// Build this provider with an Reqwest HTTP transport.
#[cfg(feature = "reqwest")]
pub fn on_reqwest_http(self, url: url::Url) -> Result<L::Provider, TransportError>
where
L: ProviderLayer<RootProvider<N, BoxTransport>, N, BoxTransport>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably on me, but we should not box here, given the user has explicitly specified the transport type to be used. replace these BoxTransport usages with naming the Http type

prestwich
prestwich reviewed Mar 29, 2024
View reviewed changes
crates/provider/src/builder.rs Outdated
Ok(self.on_client(client))
}

/// Build this provider with an Hyper HTTP transport.
#[cfg(feature = "hyper")]
pub fn on_hyper_http(self, url: url::Url) -> Result<L::Provider, TransportError>
where
L: ProviderLayer<RootProvider<N, BoxTransport>, N, BoxTransport>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

Copy link
Member

@prestwich prestwich Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may require reproducing generics from down the stack I think. if it gets ugly or annoying go ahead and skip this for hyper

This was referenced Mar 29, 2024
Merged
Merged
@prestwich
Copy link
Member

actually I'm gonna go ahead and rebase and do this :)

@prestwich prestwich merged commit c8d9819 into alloy-rs:main Mar 29, 2024
17 checks passed
@leruaa leruaa deleted the http_transport_optional branch October 10, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@prestwich prestwich prestwich approved these changes

@onbjerg onbjerg onbjerg approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Make HTTP provider optional, on by default
4 participants
@leruaa @prestwich @onbjerg @mattsse