Allow bitflags v1 and v2 to coexist#894
Conversation
|
Unfortunately this doesn't work because Library A might use defmt to describe some data type and expect the bitflags v1 API whilst Library B might use defmt but expect the bitflags v2 API (if we add it). You could have a feature flag for bitflags v2 so that we keep the dependency list as short as possible, but it has to be additional and exported as a different macro with a different name. |
705a4d2 to
aabac90
Compare
|
The CHANGELOG was reformatted. Can you rebase? |
0ffa167 to
5f6ef8a
Compare
|
The format check failed, I'm afraid. |
CHANGELOG.md
Outdated
| * [#845] `decoder`: fix println!() records being printed with formatting | ||
| * [#843] `defmt`: Sort IDs of log msgs by severity to allow runtime filtering by severity | ||
| * [#822] `CI`: Run `cargo semver-checks` on every PR | ||
| * [#877]:`defmt`: Allow `bitflags` v1 and v2 to coexist |
There was a problem hiding this comment.
- This change was made to Defmt-next because defmt-0.3.9 has already been released so it needs to be in the section above.
- This should refer to the PR, which is Allow bitflags v1 and v2 to coexist #894.
- You will need to add a URL for
[#894]down at the bottom of the file for the link to be valid.
b923d69 to
b27ffe3
Compare
CHANGELOG.md
Outdated
| > A highly efficient logging framework that targets resource-constrained devices, like microcontrollers | ||
|
|
||
| [defmt-next]: https://github.com/knurling-rs/defmt/compare/defmt-v0.3.9...main | ||
| [defmt-next]: https://github.com/knurling-rs/defmt/pull/894 |
There was a problem hiding this comment.
We already have a markdown link for [defmt-next] which was correct. You can't add another.
The change you've made needs to be listed under ## [defmt-next] heading.
b27ffe3 to
c3cca96
Compare
58cab23 to
5a22752
Compare
macros/src/items/bitflags.rs
Outdated
| let bits_access = if is_v2 { | ||
| quote! { bits() } | ||
| } else { | ||
| quote! { bits} |
There was a problem hiding this comment.
| quote! { bits} | |
| quote! { bits } |
|
@manushT - we still have an outstanding comment on this one. |
| pub use bitflags::bitflags; | ||
|
|
||
| #[cfg(feature = "bitflagsv2")] | ||
| pub use bitflagsv2::bitflags as bitflagsv2; |
There was a problem hiding this comment.
may I ask for the bitflags traits (Bits & Flags) to also be re-exported.
| pub use bitflagsv2::bitflags as bitflagsv2; | |
| pub use bitflagsv2::{bitflags as bitflagsv2, Bits, Flags}; |
I need them, and other users might want them too, for a project and would prefer relying on the version installed by defmt instead of listing it manually.
Refers to #923
There was a problem hiding this comment.
That's reasonable - perhaps for v2 import the whole crate like:
pub use bitflags::{bitflags, self};
#[cfg(feature = "bitflagsv2")]
pub use bitflagsv2::{bitflags as bitflagsv2, self};ce922bb to
fb82c87
Compare
LeonMatthesKDAB
left a comment
There was a problem hiding this comment.
Mostly looks good to me, only two nitpicks.
fb82c87 to
abd313a
Compare
abd313a to
e8f6af3
Compare
jonathanpallant
left a comment
There was a problem hiding this comment.
Minor nit on some formatting, and I like the proposal for re-exporting some useful types from the crate (to save people having to go and pull in the exact one that defmt is using).
I think this requires a Wire Format Version bump? In which case I think it's blocked until after 1.0. Maybe we should do a Version 5 with the encoded symbols and a bunch of other stuff.
6a14e99 to
a648875
Compare
a648875 to
c4e940a
Compare
|
There's still an outstanding question around importing and re-exporting the bitflags API so that people don't have to try and include the same version of bitflags in their application that defmt was using. I think that's a good idea. |
|
Any updates @manushT? |
The patch breaks cargo's additive features
Fixes #877.