-
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
move common stuff from the messages crate to shared_vcx #826
Conversation
hi @swaptr note that some of the commits in the PR are missing DCO signoff |
Yes, I am fixing that. |
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.
Just a small thing, and it is my fault for not mentioning this in the issue (I'll add it now).
The NoDecorators
name was more contextual to the messages
crate.
The type is really more of a Nothing
type from a serde standpoint (at least serde_json, which is what we care about), meaning that it's deserialized from thin air and serialized into no additional data (which is what we want in the way we use it as a dummy decorator placeholder).
So I suggest to rename it, maybe something like SerdeIgnored
and perhaps add an alias in the messages
crate to maintain the NoDecorators
naming as it's clearer in that context what it is for.
Signed-off-by: Swapnil Tripathi <swapnil06.st@gmail.com>
Signed-off-by: Swapnil Tripathi <swapnil06.st@gmail.com>
Done. I added some comments, lmk if you feel its unnecessary or verbose. |
Not at all! Comments are my spirit animal (??? 😆 ). |
Codecov Report
@@ Coverage Diff @@
## main #826 +/- ##
=======================================
Coverage 45.16% 45.17%
=======================================
Files 408 409 +1
Lines 33468 33468
Branches 7437 7439 +2
=======================================
+ Hits 15117 15119 +2
+ Misses 13583 13580 -3
- Partials 4768 4769 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Fixes #821