Skip to content

Remove the client::conn combined-version types - #2961

Closed
Michael-J-Ward wants to merge 10 commits into
hyperium:masterfrom
Michael-J-Ward:remove-combined-client-conn-types
Closed

Remove the client::conn combined-version types#2961
Michael-J-Ward wants to merge 10 commits into
hyperium:masterfrom
Michael-J-Ward:remove-combined-client-conn-types

Conversation

@Michael-J-Ward

@Michael-J-WardMichael-J-Ward commented Aug 24, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Resolves#2960

@seanmonstar ready to review, as I require your advice on the last item anyway

TODO:

  • How to handle http1::Connection::poll_without_shutdown
  • Fix failing tests
  • Cleaner http protocol specification in test
  • how to address readiness in doc for client::conn module
  • ffi::conn

@Michael-J-Ward
removes client connections that handle both http1 and http2
9743e52
See hyperium#2960
"""
The public client API for 1.0 will be the version-specific types at client::conn::{http1, http2}. The combined version may exist in hyper-util, but at least it should be removed from hyper proper. Doing so isn't particularly complex, but it will mean fixing up all the tests.
"""
@Michael-J-Ward
Updates client connection examples to explicitly specify http version
90d5834
@Michael-J-Ward
updates client tests to explicitly specify http version
7bdb9a3
@Michael-J-Ward
fixes con::http1::Conection::poll_without_shutdown
cd6f288
@Michael-J-Ward
Michael-J-Wardforce-pushed the remove-combined-client-conn-types branch from 6a4a4e8 to cd6f288CompareAugust 24, 2022 19:49
seanmonstar
seanmonstar reviewed Aug 24, 2022
View reviewed changes
Comment threadexamples/upgrades.rs Outdated
Comment threadexamples/web_api.rs Outdated
Comment threadsrc/proto/h2/client.rs
self.h2_tx.is_extended_connect_protocol_enabled()
}
}
// impl<B> ClientTask<B>

@seanmonstarseanmonstarAug 24, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this no longer needed? I think it's how HTTP/2 does it's websocket upgrades, right?

@Michael-J-WardMichael-J-WardAug 25, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

conn::http2::ClientTask is not public, and the only existing public call site is here.

hyper/src/client/conn/mod.rs

Line 370 in 9e8fc8f

ProtoClient::H2{ h2 } => h2.is_extended_connect_protocol_enabled(),

@seanmonstarseanmonstarAug 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh I see, so probably something to add back to the http2::Connection type.

Comment threadsrc/client/conn/mod.rs Outdated
@Michael-J-Ward

Michael-J-Ward commented Aug 25, 2022

Copy link
Copy Markdown
ContributorAuthor

Also - I apologize for just now seeing thee Commit Guidelines

Should I wait until after review to rebase and squash them all into a single commit?

@Michael-J-Ward
cleans protocol specfici test routing
49d4d85
This seems like the cleanes we can do without introducing a new trait or util.
@Michael-J-Ward
Michael-J-Wardforce-pushed the remove-combined-client-conn-types branch from dcb7aab to 49d4d85CompareAugust 25, 2022 12:54
@mike-wilson-hudsonmike-wilson-hudson mentioned this pull request Aug 25, 2022
Closed
@Michael-J-WardMichael-J-Ward mentioned this pull request Aug 25, 2022
Merged
@seanmonstar

seanmonstar commented Aug 25, 2022

Copy link
Copy Markdown
Member

I apologize for just now seeing thee Commit Guidelines

Not a problem at all! Also note here in PULL_REQUESTS:

If your message doesn't perfectly match the guidelines, do not worry! The person that eventually merges can easily fixup the message at that time.

@seanmonstarseanmonstar mentioned this pull request Sep 20, 2022
Merged
@seanmonstar

seanmonstar commented Sep 20, 2022

Copy link
Copy Markdown
Member

I've rebased this and started finishing the last bit in #2987.

@seanmonstar

seanmonstar commented Sep 21, 2022

Copy link
Copy Markdown
Member

Finished in #2987, thanks so much!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanmonstarseanmonstarseanmonstar left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove the client::conn combined-version types

2 participants

@Michael-J-Ward@seanmonstar