Big Trait Changes #206
Big Trait Changes #206
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #206 +/- ##
==========================================
- Coverage 87.23% 84.67% -2.56%
==========================================
Files 42 43 +1
Lines 3595 3223 -372
==========================================
- Hits 3136 2729 -407
- Misses 459 494 +35 ☔ View full report in Codecov by Sentry. |
|
Shout out to our extensive tests that caught a ton of the mistakes I was making along the way! I would have forgot to add the signature check if we didn't have a test specifically that matched the interface+member but gave a blank body. |
There was a problem hiding this comment.
I'd like to know whether the 'unchecked' should be marked unsafe as well and whether we should call it unverified, unmatched because the Result seems to suggest things are checked - which they are but not all - which may be clear to us now, but future me may not have a recollection of this.
TTWNO
force-pushed
the
checked-and-unchecked-tryfrom-msg
branch
from
c8414f0
to
1443168
Compare
|
Rebased on main. |
There was a problem hiding this comment.
This is a nice improvement over what we have.
I would like to slap a comment or warning on the unchecked and propose to rename it to something descriptive from_identified_message and add its checking sibling to the trait [try_]from_message which will do the tests. See the code review for more thoughts. I am curious to know if you think that makes sense!
| @@ -30,6 +30,32 @@ macro_rules! impl_event_properties { | |||
| }; | |||
| } | |||
|
|
|||
| /// Expands to implement From for [`crate::ObjectRef`]. | |||
There was a problem hiding this comment.
Does the crate:: become part of the the path in the rendered docs? Does the path get expanded?
If that gets tedious in the rendered docs, you can write [ObjectRef][crate::ObjectRef] (from the top of my head)
There was a problem hiding this comment.
Does the crate:: become part of the the path in the rendered docs?
Yes
Does the path get expanded?
No
Change trait bounds for auto-impl of MessageConversion
- Correct interface, member and invalid body - Correct body, interface and invalid member
The order of validation is the following when using `Event::try_from(Message)`: - MissingInterface - InterfaceMatch - MissingMember - MemberMatch - MissingSignature - SignatureMatch Or at least, that's what it should be. THe commit makes individual implementations of `TryFrom<Message>` cohere to the same order of validation.
- `validate_interface` (auto-impl) - `validate_member` (auto-impl) - `validate_body` (auto-impl) - `try_from_validated_message_parts` (required) - This constructs the event from an to it with the arguments ObjectRef,Self::Body
TTWNO
force-pushed
the
checked-and-unchecked-tryfrom-msg
branch
from
1443168
to
cb70e23
Compare
Summary
Split
BusPropertiesinto theconst &'static strs (still calledBusProperties), and message conversion:MessageConversion(TODO: name).All trivial events (where they contain only the field
item) now implementFrom<ObjectRef>.All events that implement
From<ObjectRef>get theirMessageConversionimplementation for free (blanket impl).Rename:
from_message_parts(ObjectRef, Self::Body), totry_from_message_unchecked(&zbus::Message)An additional check was added to all implementations of
From<&zbus::Message>(the macro) requireing that the body signature match the event's body signature—this supplements the xisitng checks on the interface and member.Commits
TODO
Eventand interface-grouped enums use_uncheckedinstead ofTryFrom<&Message>_uncheckedvariant deserialization for interface-grouped enums, which will not check the validity of the interface before continuing,