-
Notifications
You must be signed in to change notification settings - Fork 59
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
add optional defmt support for error types #76
Conversation
add defmt as an optional dependent package
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.
Changes look good me to, thanks for adding it! Can you add an entry in CHANGELOG.md
mentioning the change?
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.
Also, it would be good to run CI with --all-features
, so maybe update .github/workflows/ci.yml
as well if you're comfortable. If not, let me know and I'll do it :)
I hope I did the CI part properly if not feel free to amend that... |
@@ -19,6 +19,7 @@ pub type Result<T> = core::result::Result<T, Error>; | |||
/// This type represents all possible errors that can occur when deserializing JSON data | |||
#[derive(Debug, PartialEq, Eq, Clone)] | |||
#[cfg_attr(not(feature = "custom-error-messages"), derive(Copy))] | |||
#[cfg_attr(feature = "defmt", derive(defmt::Format))] |
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 think you need to add:
#[cfg_attr(feature = "defmt", defmt(defmt::Debug2Format))]
above the heapless::String<64>
error type.
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.
or adding "defmt-impl" feature into the heapless dependency
Is there anything here I can help with to get this merged? |
@vhdirk At this point, I would consider this PR abandoned. If you want to fork this current work and address my remaining comments, I'd be happy to review it as a separate PR :) |
@vhdirk if you can't do it, i can also help, tell me |
It's already being continued in #77 for now :) |
@vhdirk Thanks for picking up where I started with this and getting it in there! |
add defmt as an optional dependent package
I am only able to test defmt v0.3.0 as the rest of my project is using that version the latest is at v0.3.5
closes #75