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.

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

Switch from pin-project to pin-project-lite #151

Merged
merged 1 commit into from
Nov 16, 2021
Merged

Switch from pin-project to pin-project-lite #151

merged 1 commit into from
Nov 16, 2021

Conversation

jplatte
Copy link
Collaborator

@jplatte jplatte commented Oct 15, 2021

Fixes #115.

Contains a decent amount of duplicated code, most of which can be removed once rust-lang/rust#51085 is stable. It might be possible to reduce this duplication using a macro (but I'm not sure whether macros are allowed to take the place of entire match arms, rather than just patterns / expressions), if you want I'll look into that.

@davidpdrsn davidpdrsn added this to the 0.2.0 milestone Nov 8, 2021
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Thanks @jplatte. It looks good to me 😊

A merge/rebase of master should fix CI.

tower-http/src/compression/body.rs Show resolved Hide resolved
@davidpdrsn
Copy link
Member

Just a small heads up I think I'll merge #156 before this, which might lead to some merge conflicts here.

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 15, 2021

Rebased. Also had to switch the new cors module to pin-project-lite, but that was straight-forward.

@@ -4,7 +4,7 @@
//!
//! ```
//! use http::{Request, Response, Method, header};
//! use hyper::Body;
//! use hypefut: futureBody;
Copy link
Member

Choose a reason for hiding this comment

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

I guess this needs to be

Suggested change
//! use hypefut: futureBody;
//! use hyper::Body;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like rust-analyzer used some outdated spans when doing updating uses of a field I renamed 😅

Copy link
Member

Choose a reason for hiding this comment

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

huh, that seems like maybe something that deserves an upstream bug report?

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Yep thats cool. Thanks!

@davidpdrsn davidpdrsn enabled auto-merge (squash) November 15, 2021 13:09
auto-merge was automatically disabled November 15, 2021 13:15

Head branch was pushed to by a user without write access

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks good to me; i commented on a couple of minor notes that aren't really blocking.

tower-http/src/auth/require_authorization.rs Show resolved Hide resolved
tower-http/src/compression/body.rs Show resolved Hide resolved
tower-http/src/compression/body.rs Show resolved Hide resolved
@davidpdrsn
Copy link
Member

davidpdrsn commented Nov 16, 2021

@jplatte wanna fix the last round of conflicts? Then we can merge 🚀

@jplatte
Copy link
Collaborator Author

jplatte commented Nov 16, 2021

Done. Also added a reference to the exhaustive match feature tracking issue to the first match never_enum {} occurrence in each body.rs.

@davidpdrsn davidpdrsn enabled auto-merge (squash) November 16, 2021 15:23
@davidpdrsn davidpdrsn merged commit 371bb3f into tower-rs:master Nov 16, 2021
@davidpdrsn
Copy link
Member

Thanks a lot for working on this!

@jplatte jplatte deleted the pin-project-lite branch November 16, 2021 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to pin-project-lite
3 participants