Skip to content

Conversation

@jbr
Copy link
Member

@jbr jbr commented Nov 25, 2020

No description provided.

Copy link
Member

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

This structure strikes me as being a bit odd, see comment below.

However, along the lines of this PR, might we also want a From<Box<dyn T>>?

/// An upgraded HTTP connection.
#[derive(Debug, Clone)]
pub struct RawConnection<Inner> {
inner: Inner,
Copy link
Member

@Fishrock123 Fishrock123 Nov 25, 2020

Choose a reason for hiding this comment

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

If Connection is RawConnection<Box<dyn T>> anyways, why isn't inner just directly Box<dyn T>?

Copy link
Member

Choose a reason for hiding this comment

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

Good question; not sure. I think we should pick that up when stabilizing the upgradesubmodule.

@jbr
Copy link
Member Author

jbr commented Nov 26, 2020

This (the creation of a Connection) will only ever be used in one place probably, so it's not worth adding an Into. I think this code probably could be simplified, but that's not what this PR is about

@yoshuawuyts yoshuawuyts merged commit 3594a2a into main Nov 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the new-connection branch November 27, 2020 13:06
@yoshuawuyts yoshuawuyts mentioned this pull request Nov 27, 2020
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.

4 participants