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

Add i128 feature #52

Closed
wants to merge 4 commits into from
Closed

Add i128 feature #52

wants to merge 4 commits into from

Conversation

semyon2105
Copy link

Hi!

This PR adds an optional i128 feature which enables conversions between BigDecimal and i128/u128. It also enables deserialization support for these types.

I also had to replace the serde_json test dependency with serde_yaml because of incomplete support of i128 in serde_json. serde_json recognized numeric values as Serde maps when the arbitrary_precision feature was enabled, which broke tests. serde_yaml had no such problems.

@semyon2105
Copy link
Author

It seems like serde_yaml uses a feature which was stabilized in Rust 1.17. Can we drop support for Rust 1.16 in favor of 1.17?

@rubdos
Copy link
Contributor

rubdos commented Feb 13, 2019

I also had to replace the serde_json test dependency with serde_yaml because of incomplete support of i128 in serde_json. serde_json recognized numeric values as Serde maps when the arbitrary_precision feature was enabled, which broke tests. serde_yaml had no such problems.

Do they have a tracking issue for that? Mind linking that here?

@semyon2105
Copy link
Author

My apologies. Turns out this is not a bug but a design decision. With arbitrary_precision feature enabled, serde_json deserializes all numbers into a Number value. So there has to be an additional step: conversion from the Number value into BigDecimal. I guess I'll revert the serde_yaml dependency and add serde tests for when the arbitrary_precision feature is enabled.

We can also add an optional serde_json feature in which there will be a From<serde_json::Number> implementation for BigDecimal.

@semyon2105
Copy link
Author

Nevermind my previous comment - serde_json purposely does not implement visit_u128 and visit_i128. Here's a related PR that was rejected: serde-rs/json#498 .

We can test deserialization of 128 ints only using the serde_yaml crate

@semyon2105 semyon2105 closed this Jan 28, 2021
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.

2 participants