-
Notifications
You must be signed in to change notification settings - Fork 882
Parse Version Information Transport Parameter #2185
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
base: master
Are you sure you want to change the base?
Conversation
|
@LPardue and @ghedo -- as usual, thank you both for this amazing implementation! I have added support for encoding/parsing the Version Information Transport Parameter. I realize there are additional changes that could be incorporated into this PR (namely, using the Version Information in the process of version negotiation) and I would be more than happy to talk that. But, I didn't want to get too far ahead in case this PR was unwelcome or you already had something planned. If you think that what is here so far is reasonable and would like me to press on, just give me the nod! I would love to help however I can! |
|
Thanks for the PR Will. I think its probably ok to parse the received TP for debug/analysis reasons. However, adding full version negotiation support is a bit more work and something that has no been a high priority for us. It would probably be best to keep this PR focuses just on parsing received TPs and not provide an API to allow apps to set them (and hence no encoding). That would let us land the PR soonish. Then follow up work could consider a more-complete version negotiation approach, which will need lots of testing etc |
Funny that is your suggestion, because that is my preferred plan, too!! As you know, my use case is parsing TPs, so this will do exactly what I want. I will modify the PR (according to this plan and the helpful suggestion above) and resubmit! Thanks, as usual! |
45b6494 to
cea4d1a
Compare
|
Let me know if you think that this version is better! |
|
I don't want to overstep, but if you would like me to add to the PR an update to the minimum supported Rust version (to solve this failure: 45b6494902ce0dc40fe8543f6d42f36385cd0e1e), I would be more than happy to do that! |
Yeah a separate PR to bump th MSRV to 1.83.0 would be welcomed! |
e6383a5 to
4565692
Compare
Let me know if #2197 helps! |
c0e8f32 to
d11097c
Compare
d11097c to
464948c
Compare
464948c to
c0e664a
Compare
c0e664a to
32f2430
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.
Thanks for the change!
32f2430 to
0cee383
Compare
0cee383 to
63a5476
Compare
|
Hope that the additional tests are helpful! |
Does not (yet!) incorporate parsed Version Information Transport Parameter into version negotiotiation.