search

rust-embedded / nb

Minimal and reusable non-blocking I/O layer
Apache License 2.0
86 stars 15 forks source link

feat: Add defmt as optional dependency #39

Closed elpiel closed 1 year ago

elpiel commented 1 year ago

A small PR for impl defmt::Format for Error when E also impls defmt::Format

eldruin commented 1 year ago

nb is considered stable starting with its 1.0 release. Merging this would mean we would depend on a non-stable dependency. See: Rust API guidelines/C-STABLE

elpiel commented 1 year ago

There are crates that define unstable features but I agree that we can go without this PR. There is still the defmt::Debug2Format which can be used for such occasions although it's not ideal when the nb crate does not derive(Format).

Thanks anyway.

Rahix commented 1 year ago

Just my two cents: I think an unstable optional dependency doesn't violate C-STABLE at all. It doesn't influence users who restrict themselves to stable dependencies in any way. And users of defmt already live in unstable land so it is fine for them. And of course it would help these users immensely if such a feature is made available.

I think the only question is how to make it obvious that the feature is unstable. Maybe naming the feature flag unstable-defmt or just leaving a comment about this in Cargo.toml.

eldruin commented 1 year ago

@Rahix Thanks for the feedback. I suggest we discuss this in today's WG meeting.

Rahix commented 1 year ago

Ah, unfortunately I won't be able to make it to the meeting today :/ It'd be fine for me if you want to discuss this with the others regardless. I don't have strong feelings about it, just thought it deserves a bit more discussion maybe.

adamgreig commented 1 year ago

We had a brief discussion at the meeting today and concluded it should be fine to support this, behind a feature name like defmt-0.3 (both to indicate it's unstable, and in case we later want to add support for a future defmt version without removing this one).

adamgreig commented 1 year ago

@elpiel, would you mind renaming the feature to "defmt-0.3"?

elpiel commented 1 year ago

No, I don't have anything against it. Will do it tomorrow. Thanks for the updates!

elpiel commented 1 year ago

@adamgreig I've updated the PR. Sadly, I had to add quotes around the name of the feature as it uses a ..

elpiel commented 1 year ago

I've update the CI workflow + bumped actions/checkout to v3 and added docs to lib.rs for the unstable feature.

bors[bot] commented 1 year ago

Timed out.

Dirbaio commented 1 year ago

bors.toml needs updating as well

elpiel commented 1 year ago

Done @eldruin

bors[bot] commented 1 year ago

Build succeeded: