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

tcp: unexport TcpTransport.Upgrader #1596

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Jun 11, 2022

All that users care about is that the transport constructed is a tpt.Transport, they don't care about the (incorrectly capitalised) TcpTransport.

This PR doesn't solve any particular issue, but it's part of a cleanup of our exposed API. As a reference point, the QUIC transport doesn't expose any transport either.

@Stebalien
Copy link
Member

This was intentional. The convention is to have constructors return a concrete, public type. That lets us add additional methods, expose the upgrader, etc. in case the user wants to change TCP specific options.

That's less of an issue now that the transport takes functional options, but it's still convenient to be able to expose custom methods/metadata on transports.

@marten-seemann
Copy link
Contributor Author

Fair enough. I don't think we're using it, but it might be useful at some point.

I disagree regarding the upgrader though. Modifying that member variable will always be racy, so the only thing you can safely do is a read access. If we ever need to give access to that, a getter function would be the better API.

@Stebalien
Copy link
Member

I disagree regarding the upgrader though. Modifying that member variable will always be racy, so the only thing you can safely do is a read access. If we ever need to give access to that, a getter function would be the better API.

Yeah, you're probably right. IIRC we did that due to a limitation in our libp2p constructor (which is no longer an issue).

Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

If we don't need to expose this and we can minimize the api surface then it seems like a win :)

@marten-seemann marten-seemann changed the title tcp: unexport the TcpTransport tcp: unexport TcpTransport.Upgrader Jun 23, 2022
@marten-seemann
Copy link
Contributor Author

I removed the last commit, so this is now just making TcpTransport.Upgrader private.

@marten-seemann marten-seemann merged commit 56cc37a into master Jun 23, 2022
@marten-seemann marten-seemann deleted the unexport-tcp-transport branch June 26, 2022 11:02
@MarcoPolo MarcoPolo mentioned this pull request Jul 7, 2022
41 tasks
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
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.

3 participants