Skip to content

Conversation

Ameobea
Copy link

@Ameobea Ameobea commented Mar 17, 2018

When dealing with arbitrary inputs such as those from serde_json::Value, making use of the existing backwards compatibility APIs can be difficult or impossible. This PR introduces the ability to cast unsigned integers to i32 using the nightly try_from API. It exposes this behind an opt-in feature and doesn't make any backwards-incompatible changes.

If you'd rather avoid this conversion strategy and stick to strict type checking, that's fine; I'm making use of these for my own project and understand if you'll want to keep them out of upstream.

I also changed all instances of try!() to ?.

Ameobea added 4 commits March 16, 2018 20:14
 * Uses unstable `TryFrom` API to convert unsigned integers to `i32`
 * Expose this functionality behind an opt-in feature gate, defaulting to the original behaviour of erroring out when encountering signed integers
 * Add tests for this functionality, but they're currently not set up to actually test it since it's behind the feature gate and there's no way that I know of to set those dynamically
@zonyitoo
Copy link
Contributor

LGTM. cc @kyeah

@kyeah
Copy link
Contributor

kyeah commented Mar 21, 2018

Hi, sorry for the delay @Ameobea, @zonyitoo!

My intuition is that users should opt into these conversions for unsigned fields, in the same way that we have this compat module for de/serializing unsigned integers: https://github.com/zonyitoo/bson-rs/blob/master/src/compat/u2f.rs.

That is, we should add to_i32 and from_i32 conversions (along with _i64) instead of adding a global conversion flag — and if we do decide on a global flag, we should provide options for TryInto<i64> and TryInto<f64>.

What do y'all think?

@zonyitoo
Copy link
Contributor

Yeah, you're right, that would be better.

@kyeah
Copy link
Contributor

kyeah commented Jul 4, 2018

Hi @Ameobea! This question popped up again in #94, and it was noted that a global feature switch is useful to have when dealing with generated code.

#95 was opened and merged before I remembered that this was still open — sorry! I'm going to close this for now since the feature has been merged as u2i, but the try! conversions are still useful, and would be welcome in a separate PR.

Note that the u2i implementation is a little different — in particular I believe it converts u64 to i64 if possible.

Thank you for putting this PR together!

@kyeah kyeah closed this Jul 4, 2018
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.

3 participants