-
Notifications
You must be signed in to change notification settings - Fork 147
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
refactor framing_sv2
example
#1468
refactor framing_sv2
example
#1468
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1468 +/- ##
=======================================
Coverage 21.92% 21.92%
=======================================
Files 135 135
Lines 9573 9573
=======================================
Hits 2099 2099
Misses 7474 7474
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7d24f3f
to
52e824f
Compare
framing_sv2
exampleframing_sv2
example
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.
Looks good. do we actually run this in the ci?
nice catch, we didn't but I just added a commit that runs it from |
Should we also add other examples that we recently included during our documentation efforts, since not all of them have been added to CI? |
|
||
// Example message type (e.g., SetupConnection) | ||
const MSG_TYPE: u8 = 1; | ||
// Example extension type (e.g., a standard Sv2 message) | ||
const EXT_TYPE: u16 = 0x0001; | ||
|
||
#[derive(Serialize, Debug)] | ||
#[derive(Serialize, Deserialize, Debug, Clone)] | ||
pub struct CustomMessage { |
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.
thinking out loud here, wouldn't it be more beneficial to actually have an SV2 message here? for the MSG_TYPE, SETUP_CONNECTION is used, maybe here as well?
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.
rationale here is to limit this example to the scope of framing_sv2
while highlighting the assumption that the crate is agnostic to which messages actually exist in the Sv2 protocol
it's meant to showcase that as long as a message is defined this way, framing_sv2
will be able to transmit it over the wire (without any kind of assertion of whether this message is part of the spec or not)
cd6226b
to
cb2b67b
Compare
This should be good to go now? cc: @plebhash |
I think this PR is ready to go since it received 2 approvals. Could you try to rebase it @plebhash ? |
cb2b67b
to
5de3953
Compare
we need to assert that the deserialized message has the same content from the original
5de3953
to
a999706
Compare
close #1467