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. |
manushT
force-pushed
the
allow-bitflags-v1-and-v2-to-coexist
branch
4 times, most recently
from
705a4d2 to
aabac90
Compare
|
The CHANGELOG was reformatted. Can you rebase? |
manushT
force-pushed
the
allow-bitflags-v1-and-v2-to-coexist
branch
2 times, most recently
from
0ffa167 to
5f6ef8a
Compare
|
The format check failed, I'm afraid. |
CHANGELOG.md
Outdated
| @@ -69,6 +69,7 @@ We have several packages which live in this repository. Changes are tracked sepa | |||
| * [#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.
manushT
force-pushed
the
allow-bitflags-v1-and-v2-to-coexist
branch
2 times, most recently
from
b923d69 to
b27ffe3
Compare
CHANGELOG.md
Outdated
| @@ -27,6 +27,7 @@ We have several packages which live in this repository. Changes are tracked sepa | |||
| > 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.
manushT
force-pushed
the
allow-bitflags-v1-and-v2-to-coexist
branch
from
b27ffe3 to
c3cca96
Compare
manushT
force-pushed
the
allow-bitflags-v1-and-v2-to-coexist
branch
3 times, most recently
from
58cab23 to
5a22752
Compare
| @@ -361,11 +361,21 @@ pub use defmt_macros::timestamp; | |||
| /// const ABC = Self::A.bits | Self::B.bits | Self::C.bits; | |||
| /// } | |||
| /// } | |||
| /// | |||
| /// #[cfg(feature = "bitflagsv2")] | |||
There was a problem hiding this comment.
I think we need some more documentation here to explain why we have two bitflags macros now and how users should pick between them. And actually, currently bitflagsv2 is completely undocumented because the new docs were added to the block for bitflags. You can check with cargo doc -p defmt --features=bitflagsv2 --open.
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. |
Urhengulas
added
the
pr waits on: author
| @@ -8,6 +8,9 @@ use crate::{Format, Formatter, Str}; | |||
| pub use self::integers::*; | |||
| 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
manushT
force-pushed
the
allow-bitflags-v1-and-v2-to-coexist
branch
3 times, most recently
from
ce922bb to
fb82c87
Compare
defmt/src/lib.rs
Outdated
| /// ``` | ||
| pub use defmt_macros::bitflags; | ||
|
|
||
| /// Generates a bitflagsv2 structure that can be formatted with defmt. Users of the defmt crate can |
There was a problem hiding this comment.
This makes it sound like bitflagsv2 is a different crate to bitflags, which is not the case, it's the same bitflags crate, but just using version 2.X.X.
Please try to formulate this clearer.
defmt/src/lib.rs
Outdated
| /// API with the replacement of the unsafe from_bits_unchecked method by the safe from_bits_retain method | ||
| /// and enhanced serialization support via an optional serde feature. | ||
| /// | ||
| /// This macro is a wrapper around the [`bitflags!`][bitflagsv2] macro of the bitflags! version 2 crate, and provides an (almost) identical |
There was a problem hiding this comment.
Nit pick: the macro name can have an ! in it, the crate should not. So please change bitflags! version 2 crate to bitflags version 2 crate.
The bitflags! macro can keep the !
defmt/src/lib.rs
Outdated
| @@ -367,6 +367,42 @@ pub use defmt_macros::timestamp; | |||
| /// ``` | |||
| pub use defmt_macros::bitflags; | |||
|
|
|||
| /// Users of the defmt crate can enable the bitflagsv2 feature by adding defmt = { features = ["bitflagsv2"] } | |||
There was a problem hiding this comment.
Nit: On the overview page, Rustdocs will list the first paragraph as the high-level description.
This paragraph is a little bit long for such a description, please add a short description like there is for the v1 macro
e.g.:
/// Generates a bitflags structure that can be formatted with defmt (using version 2 of the bitflags crate)
///
fb82c87 to
abd313a
Compare
The patch breaks cargo's additive features
Fixes #877.