chore: Review fixes #458
chore: Review fixes #458
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/replies #458 +/- ##
================================================
- Coverage 72.37% 72.12% -0.26%
================================================
Files 63 63
Lines 3823 3860 +37
================================================
+ Hits 2767 2784 +17
- Misses 1056 1076 +20 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -11,7 +11,36 @@ use crate::types::msg_field::MsgField; | |||
| use crate::types::msg_variant::{MsgVariant, MsgVariants}; | |||
| use crate::utils::emit_turbofish; | |||
|
|
|||
| const NUMBER_OF_DATA_FIELDS: usize = 1; | |||
| const NO_ALLOWED_DATA_FIELDS: usize = 1; | |||
There was a problem hiding this comment.
NO stands for number here? If so it's quite misleading in this context
There was a problem hiding this comment.
ye, number of. It is a bit if you don't check the type. I will revert this change.
| fn first_reply( | ||
| &self, | ||
| _ctx: ReplyCtx, | ||
| #[sv::data(opt, raw)] _data: Option<Binary>, |
There was a problem hiding this comment.
Let's add few more combinations:
- What would happen if there is an additional argument before
sv::data? - What happen if we use
#[sv::data(opt, raw)]onctx: ReplyCtx? - What happen if we use
#[sv::payload(raw)]onctx: ReplyCtx? - What if we skip
ctx: ReplyCtx? - Can we use
#[sv::data(raw)]after#[sv::payload(raw)]?
There was a problem hiding this comment.
- Missing payload parameter. - This might cause some weird behavior due to deserialization to empty tuple.
Good points.
For 2 and 3 nothing would happen as context types are skipped during parsing.
For 4 as in case of every message type it would fail with Rust compile error that context types couldn't be converted to whatever parameter is defined first. We are unable to check if the second parameter in the method signature is ReplyCtx as this would make usage of aliases impossible or very restricted and in the end wrong type could still be passed there.
Added test cases for the rest.
| @@ -0,0 +1,8 @@ | |||
| error: Missing payload parameter. | |||
There was a problem hiding this comment.
That's not 100% accurate - there is a payload parameter
There was a problem hiding this comment.
Actually there is a payload attribute, not parameter ^^
But yeah, it's not 100% obvious, but it's such a niche error, I believe, that I thought it would be sufficient.
I will add check that no sv attributes were used on the self and ctx parameters.
No description provided.