-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
expose the QUIC version of a connection #3620
expose the QUIC version of a connection #3620
Conversation
4c857ab
to
904a522
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's call this Version
instead of QUICVersion
.
Codecov ReportBase: 85.49% // Head: 85.48% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3620 +/- ##
==========================================
- Coverage 85.49% 85.48% -0.01%
==========================================
Files 142 142
Lines 10294 10295 +1
==========================================
Hits 8800 8800
- Misses 1109 1110 +1
Partials 385 385
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but CI is unhappy (not gofmt-ed).
Should we make the version numbers non-internal? https://github.com/lucas-clemente/quic-go/blob/master/internal/protocol/version.go#L19 |
It already is, there's an alias in the quic package. We should probably use that one. |
ah I can't believe I didn't see that at the top of the file. Does this PR win for most back and forths relative to the code change? |
We already track the QUIC version in a connection. This simply exposes this in the public interface.