Skip to content

Conversation

@hawkinsw
Copy link
Contributor

@hawkinsw hawkinsw commented Oct 8, 2025

Does not (yet!) incorporate parsed Version Information Transport Parameter into version negotiotiation.

@hawkinsw hawkinsw requested a review from a team as a code owner October 8, 2025 22:35
@hawkinsw
Copy link
Contributor Author

hawkinsw commented Oct 8, 2025

@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!

@LPardue
Copy link
Contributor

LPardue commented Oct 13, 2025

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

@hawkinsw
Copy link
Contributor Author

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!

@hawkinsw hawkinsw force-pushed the version_information branch from 45b6494 to cea4d1a Compare October 13, 2025 22:31
@hawkinsw hawkinsw requested a review from LPardue October 13, 2025 22:33
@hawkinsw
Copy link
Contributor Author

Let me know if you think that this version is better!

@hawkinsw
Copy link
Contributor Author

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!

@LPardue
Copy link
Contributor

LPardue commented Oct 15, 2025

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!

@hawkinsw hawkinsw force-pushed the version_information branch 2 times, most recently from e6383a5 to 4565692 Compare October 15, 2025 16:50
@hawkinsw
Copy link
Contributor Author

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!

Let me know if #2197 helps!

@hawkinsw hawkinsw force-pushed the version_information branch 6 times, most recently from c0e8f32 to d11097c Compare October 15, 2025 19:12
@hawkinsw hawkinsw force-pushed the version_information branch from d11097c to 464948c Compare October 15, 2025 22:33
@LPardue LPardue force-pushed the version_information branch from 464948c to c0e664a Compare October 16, 2025 12:59
@LPardue LPardue changed the title Encode And Parse Version Information Transport Parameter Parse Version Information Transport Parameter Oct 16, 2025
@hawkinsw hawkinsw force-pushed the version_information branch from c0e664a to 32f2430 Compare October 18, 2025 05:59
@hawkinsw hawkinsw requested a review from LPardue October 22, 2025 01:19
Copy link
Contributor

@antoniovicente antoniovicente left a 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!

@hawkinsw hawkinsw force-pushed the version_information branch from 32f2430 to 0cee383 Compare October 25, 2025 20:51
@hawkinsw
Copy link
Contributor Author

Hope that the additional tests are helpful!

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