Skip to content
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

Support ResponseType comparisons and make discriminant of ResponseType match CoAP response value #42

Open
kach-sj opened this issue Jan 13, 2025 · 2 comments

Comments

@kach-sj
Copy link

kach-sj commented Jan 13, 2025

For manipulation of ResponseTypes it would be useful to

  1. Derive PartialOrd over ResponseType so that consumers can compare entire error categories e.g. check if response type is an error.
  2. Implement a built-in function like is_error(). Currently if a user wants to determine this, the most elegant version would still have to do a conversion to MessageClass:
fn is_error(response_type: &ResponseType) -> bool {
    u8::from(MessageClass::Response(*response_type)) >= 0x80
}
  1. Have the discriminant of ResponseType be an integer code value. This could be a direct translation of the response code value in hex and could lift the translation layer into the enum itself. Sample translation - https://docs.nordicsemi.com/bundle/sdk_nrf5_v16.0.0/page/group_iot_sdk_coap_codes.html.
@martindisch
Copy link
Owner

That makes sense. Two questions then:

  1. I'm hesitant to implement your third request by assigning discriminants directly, as that would break the public API. I'd much rather introduce an impl From<ResponseType> for u8 conversion, as is already the case for several other types. Does that work for you?
  2. Suppose we then also add is_error() on the type, do you still need the PartialOrd implementation? Because you now have the facilities to determine if it's an error, and if you still want to do range requests you can do that on the u8 obtained by the newly implemented numeric conversion.

@kach-sj
Copy link
Author

kach-sj commented Jan 14, 2025

  1. Ah yes, that makes sense.
  2. I think that's fair. I'm not aware of a downside to implementing PartialOrd, so in my opinion it would be nicer to have the more ergonomic comparison response_type >= ResponseType::BadRequest rather than u8::from(response_type) >= u8::from(ResponseType::BadRequest) though. This would be especially helpful when checking response code ranges that are not just errors e.g. between 400 and 500 responses.

Thanks for considering!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants