martindisch / coap-lite

A lightweight CoAP message manipulation crate, ideal for embedded environments
Other
26 stars 18 forks source link

Fix wrong check on observers retaintion #14

Closed jiayihu closed 3 years ago

jiayihu commented 3 years ago

I actually had this commits for a long while, but wanted to check to be sure it worked correctly, then forgot to ever open the PR.

martindisch commented 3 years ago

Good to hear from you again! I made some changes that I'd like you to review (you might have to git fetch && git reset --hard origin/master since I rebased) and have some questions too:

  1. In https://github.com/martindisch/coap-lite/pull/14/commits/105dc977e99673a9a3ad9c6f019b3d2ff37d91e3 I added a short description to the new function you introduced so it's not the only publicly exposed functionality without documentation. Is what I wrote correct or should it be something else?
  2. In https://github.com/martindisch/coap-lite/pull/14/commits/412b310c11b3e5d9294d8a9f18a9a89675aef73b I removed a #[macro_use] that Clippy told me was unnecessary. Could you test that this doesn't break anything, even when using the log feature?
  3. Is there a reason why in the case where the log feature isn't activated the macro does let _ = $arg; in order to turn it into a no-op instead of actually doing nothing (e.g. {})?
  4. Do you think it would be useful to have an explicit feature for log? Something like
    [features]
    with-log = ["log"]

    It's not technically necessary since having an optional dependency adds an implicit feature with the same name, but it would make it more obvious to potential users that the functionality exists. I don't mind either way, you decide.

martindisch commented 3 years ago

I'm ready to merge this as soon as you give my changes the green light. Just a small ping in case this slipped through the cracks, I'm in no rush at all :-)

jiayihu commented 3 years ago

Sorry, time flies so fast these days! 1 and 2 okay 3 The honest answer is that I don't know. Macro definitions are still a mystery to me 😄 The implementation was taken from https://github.com/smoltcp-rs/smoltcp/blob/master/src/macros.rs IIRC 4 I think it would make sense for better discoverability, but smoltcp doesn't do it. I guess we can assume one knows about the implications of optional = true.

Thanks for the patience, it really slipped through my mind :)

martindisch commented 3 years ago

Macro definitions are still a mystery to me

Same here ;-) Guess we'll leave it like that, it doesn't hurt and the compiler removes it anyway. Thank you.