feat: Add auto deserialization of reply data (#445) #448
Conversation
jawoznia
force-pushed
the
jawoznia/feat/data_deserialization
branch
from
8c509ad to
e42fc9e
Compare
jawoznia
force-pushed
the
jawoznia/feat/data_deserialization
branch
from
e42fc9e to
c580365
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/replies #448 +/- ##
================================================
+ Coverage 71.95% 72.49% +0.53%
================================================
Files 59 62 +3
Lines 3634 3795 +161
================================================
+ Hits 2615 2751 +136
- Misses 1019 1044 +25 ☔ View full report in Codecov by Sentry. |
| // If the `data` attribute is missing, the data field should be omitted. | ||
| _data: Option<Binary>, |
There was a problem hiding this comment.
I don't really get i t - why exactly the _data is not now interpreted as a part of a payload? I am not sure about this being wrong in any way. Maybe we could warn here that this looks like a missing attribute, but I don't think it's an error.
There was a problem hiding this comment.
You are right. I missed test for missing sv::data parameter. It is impossible to consistently detect missing sv::data attribute.
sylvia/tests/reply_data.rs
Outdated
| } | ||
| } | ||
|
|
||
| mod tests { |
There was a problem hiding this comment.
I'm wondering if there is point to have additional module here? Not a strong opinion, but seems obsolete.
There was a problem hiding this comment.
It's not needed. It's mostly to separate the imports, but it doesn't benefit us really,
| fn data( | ||
| &self, | ||
| _ctx: ReplyCtx, | ||
| #[sv::data] _data: String, |
There was a problem hiding this comment.
I'm wondering about one test with a bit more complex type than a String that goes through serialization process.
sylvia/tests/reply_data.rs
Outdated
| // MultiTest version is released. | ||
| // Payload is not currently forwarded in the MultiTest. | ||
| // _instantiate_payload: InstantiatePayload, | ||
| #[sv::payload] _payload: Binary, |
There was a problem hiding this comment.
I think it was supposed to be #[sv::payload(raw)], right? Or is it a part of different PR (that would make sense as this is the data, not payload one).
There was a problem hiding this comment.
It should be done as part of this PR. Thank you for pointing this out.
| }; | ||
| }, | ||
| _ => { | ||
| emit_error!(self.name().span(), "Invalid data usage."; |
There was a problem hiding this comment.
I am wondering if there is a way to include the error testing we have in ui to the cov report... We could technically do the #[tarpaulin(ingore)] here but this then doesn't check if the path is ever tested... Just leaving as an idea, maybe we could create issue for this, idk.
There was a problem hiding this comment.
We have an issue for this #339. Unfortunately I don't see a way to do that since trybuild runs its own compilations. If we would figure out something there is an issue to track that though.
Expect `(raw)` parameter in the `sv::payload` attribute.
jawoznia
force-pushed
the
jawoznia/feat/data_deserialization
branch
from
36e3267 to
30da5ec
Compare
No description provided.