-
Notifications
You must be signed in to change notification settings - Fork 83
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
feature: Derive Display for all aries messages #1069
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1069 +/- ##
=====================================
Coverage 0.05% 0.05%
=====================================
Files 471 471
Lines 24009 24009
Branches 4306 4294 -12
=====================================
Hits 13 13
Misses 23995 23995
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8b2bf0f
to
0565c52
Compare
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.
I can't comment on display_as_json
code, as that's beyond my understanding, but other than that, LGTM!
aries/messages/src/msg_parts.rs
Outdated
#[builder(build_method(vis = "", name = __build))] | ||
pub struct MsgParts<C, D = NoDecorators> { | ||
pub struct MsgParts<C, D> { |
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.
Can you please specify the problem that the default type was causing?
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.
Prompted me to dig into this more - the issue was the proc macro was too simplistic, it was generating stuff like
impl<C, D = NoDecorators> std::fmt::Display for MsgParts<C, D>
where
C: serde::Serialize,
D: serde::Serialize,
while is should had been
impl<C, D> std::fmt::Display for MsgParts<C, D>
where
C: serde::Serialize,
D: serde::Serialize,
This is addressed now, syn crate has useful function split_for_impl
available for Generics
type, which allows to work with the generics related statements in more granular fashion.
So I am re-introducing the default NoDecorators type, reverting some of the changes done here
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.
I hope that the fact that the decision to rather modify the sites of usage instead of fixing the macro seemed as a better approach is more about due to skill issue rather than the fact that procedural macros are on harder to maintain :) Probably both, though...
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.
@mirgee I think you misunderstood me, I fixed the macro. No change to callsites anymore
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.
@Patrik-Stas No, I am talking about the initial decision (hence the past tense there).
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.
Ah yeah. In fact I misread. Indeed exactly as you said, it's definitely objectively better to make the macro more robust.
8c4bea2
to
68f2df7
Compare
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
68f2df7
to
a861b63
Compare
Display
trait for all aries messagesD = NoDecorators
- turned out somewhat problematic in conjunction with the customDisplay
proc macro. Not a big price to pay though, only slightly inconveniences developer working inmessages
crate, but notmessages
consumers.