Skip to content

Conversation

@dekellum
Copy link
Contributor

@dekellum dekellum commented Dec 11, 2020

As suggested in #230, I added TryFrom <usize>, <u32> and <u64> and From<StatusCode> for the same integer types, and re-implemented <u16> with the same macro. Oddly enough, if I stop with unsigned types, then previously working doctests for response::{Response, Builder} start failing with the following:

error[E0277]: the trait bound `StatusCode: From<i32>` is not satisfied
 --> src/response.rs:537:6
  |
7 |     .status(200)
  |      ^^^^^^ the trait `From<i32>` is not implemented for `StatusCode`
  |
  = help: the following implementations were found:
            <StatusCode as From<&'a StatusCode>>
  = note: required because of the requirements on the impl of `Into<StatusCode>` for `i32`
  = note: required because of the requirements on the impl of `TryFrom<i32>` for `StatusCode`

...which seems almost more like an obscure rustc or stdlib bug? (Fails on MSRV 1.39.0 as well as a recent nightly) (edit: see comment below). As this was done with macro_rules!, its easy to extend to the signed types i16, i32, isize, and i64 types to avoid the above issue. Since its implemented via u16::try_from there is nothing particularly more unusual about including support for the signed types: negative values are just another case of InvalidStatusCode.

Also added bi-directional PartialEq for all of these integer types. This makes it so u16 doesn't have any particular favor in the public interface other then the existing StatusCode::from_u16 and as_u16. If a user sticks to TryInto, From and PartialEq then u16 (and NonZeroU16) is just a private implementation detail.

The PR includes some misc related test and doc improvements.

Given the warning of the about mentioned compile error, I would not suggest merging this for release in a patch release like 0.2.2. It warrants at least a MINOR bump 0.3.0 or 1.0.0.

Fixes #230

@dekellum
Copy link
Contributor Author

Asked about the error mentioned above and got a helpful, quick answer here:

https://users.rust-lang.org/t/whats-so-special-about-the-i32-type-here/52613/3

So its known limitation of the compiler and i32 conversions need to be implemented to handle the i32 fallback for non-inferred integer literals. Inference no longer happens with the multiple TryFrom impls of this PR.

@dekellum dekellum force-pushed the status-integer-conversions branch from f1a1ff6 to cdbf9a5 Compare December 11, 2020 17:32
@dekellum dekellum force-pushed the status-integer-conversions branch from cdbf9a5 to 6fbf565 Compare December 11, 2020 17:36
@dekellum
Copy link
Contributor Author

dekellum commented Dec 15, 2020

@kaj how would this play out with #442, #454? Could you cross review?

@kaj
Copy link

kaj commented Dec 15, 2020

@kaj how would this play out with #442, #454? Could you cross review?

I think there is no relevant conflict. #442 has Builder2::status(code) which takes a StatusCode rather than a TryInto<StatusCode>, so it doesn't really matter to that which types implement TryInto<StatusCode>. I might amend it to also proivde a Builder2::try_status(...) method that taks a TryInto<StatusCode>, but I don't think that is a conflict either. Of course, if this i merged first and there is a conflict, I'll be happy to resolve it.

Also, I think this change looks good in itself (but I'm not a maintainer of this crate).

@dekellum
Copy link
Contributor Author

Okay, thanks. That was the kind of sanity check on this PR I was hoping for.

Copy link

@robjtede robjtede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the reasoning to add these impls is sound enough

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

Successfully merging this pull request may close these issues.

Should conversions to StatusCode be implemented on more num types?

3 participants