Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Hide numeric types #64

Merged
merged 14 commits into from
Apr 25, 2017
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ extern crate yaml_rust;

pub use self::de::{from_reader, from_slice, from_str};
pub use self::ser::{to_string, to_vec, to_writer};
pub use self::value::{Sequence, Value, from_value, to_value};
pub use self::value::{Sequence, Value, from_value, to_value, Number};
pub use self::error::{Error, Result};
pub use self::mapping::Mapping;

Expand Down
37 changes: 26 additions & 11 deletions src/value/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ use serde::de::{
VariantAccess,
Visitor,
};
use num_traits::NumCast;

use super::Value;
use mapping::Mapping;
use error::Error;
use value::number::Number;

impl<'de> Deserialize<'de> for Value {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
Expand All @@ -41,22 +41,19 @@ impl<'de> Deserialize<'de> for Value {
fn visit_i64<E>(self, i: i64) -> Result<Value, E>
where E: SError,
{
Ok(Value::I64(i))
Ok(Value::Number(i.into()))
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer this to be consistent across all three number methods. Here one uses Into::into, one uses Number::from, and one uses Number::from_f64.

}

fn visit_u64<E>(self, u: u64) -> Result<Value, E>
where E: SError,
{
match NumCast::from(u) {
Some(i) => Ok(Value::I64(i)),
None => Ok(Value::String(u.to_string())),
}
Ok(Value::Number(Number::from(u)))
}

fn visit_f64<E>(self, f: f64) -> Result<Value, E>
where E: SError,
{
Ok(Value::F64(f))
Ok(Value::Number(Number::from_f64(f)))
}

fn visit_str<E>(self, s: &str) -> Result<Value, E>
Expand Down Expand Up @@ -128,8 +125,17 @@ impl<'de> Deserializer<'de> for Value {
match self {
Value::Null => visitor.visit_unit(),
Value::Bool(v) => visitor.visit_bool(v),
Value::I64(i) => visitor.visit_i64(i),
Value::F64(f) => visitor.visit_f64(f),
Value::Number(ref n) => {
Copy link
Owner

Choose a reason for hiding this comment

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

This does not need to use ref. We own the number.

if let Some(u) = n.as_u64() {
Copy link
Owner

Choose a reason for hiding this comment

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

This should use Number as Deserializer<'de> like serde_json does. That way you avoid the if / else if / else if / else unreachable mess.

Value::Number(n) => n.deserialize_any(visitor)

visitor.visit_u64(u)
} else if let Some(i) = n.as_i64() {
visitor.visit_i64(i)
} else if let Some(f) = n.as_f64() {
visitor.visit_f64(f)
} else {
unreachable!("unexpected number")
}
}
Value::String(v) => visitor.visit_string(v),
Value::Sequence(v) => {
let len = v.len();
Expand Down Expand Up @@ -420,8 +426,17 @@ impl Value {
match *self {
Value::Null => Unexpected::Unit,
Value::Bool(b) => Unexpected::Bool(b),
Value::I64(i) => Unexpected::Signed(i),
Value::F64(f) => Unexpected::Float(f),
Value::Number(ref n) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to implement this as n.unexpected() to remove the unreachable branch.

if let Some(u) = n.as_u64() {
Unexpected::Unsigned(u)
} else if let Some(i) = n.as_i64() {
Unexpected::Signed(i)
} else if let Some(f) = n.as_f64() {
Unexpected::Float(f)
} else {
unreachable!("unexpected number")
}
}
Value::String(ref s) => Unexpected::Str(s),
Value::Sequence(_) => Unexpected::Seq,
Value::Mapping(_) => Unexpected::Map,
Expand Down
7 changes: 4 additions & 3 deletions src/value/from.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::Value;
use mapping::Mapping;
use value::number::Number;

// Implement a bunch of conversion to make it easier to create YAML values
// on the fly.
Expand All @@ -9,7 +10,7 @@ macro_rules! from_integer {
$(
impl From<$ty> for Value {
Copy link
Owner

Choose a reason for hiding this comment

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

This implementation now works for f32 and f64 too, no need for a separate From<f32> and From<f64> below.

fn from(n: $ty) -> Self {
Value::I64(n as i64)
Value::Number(Number::from(n))
}
}
)*
Expand All @@ -18,7 +19,7 @@ macro_rules! from_integer {

from_integer! {
i8 i16 i32 i64 isize
u8 u16 u32 usize
u8 u16 u32 u64 usize
}

impl From<f32> for Value {
Expand Down Expand Up @@ -57,7 +58,7 @@ impl From<f64> for Value {
/// # }
/// ```
fn from(f: f64) -> Self {
Value::F64(f)
Value::Number(Number::from_f64(f))
}
}

Expand Down
14 changes: 7 additions & 7 deletions src/value/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::ops;

use super::Value;
use mapping::Mapping;
use value::number::Number;

// The original implementation of the indexing tricks below is from `serde_json`.

Expand Down Expand Up @@ -32,14 +33,14 @@ impl Index for usize {
fn index_into<'v>(&self, v: &'v Value) -> Option<&'v Value> {
match *v {
Value::Sequence(ref vec) => vec.get(*self),
Value::Mapping(ref vec) => vec.get(&Value::I64(*self as i64)),
Value::Mapping(ref vec) => vec.get(&Value::Number(Number::from(*self as i64))),
Copy link
Owner

Choose a reason for hiding this comment

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

This casting is lossy and no longer necessary. Since Number is opaque, and if the implementation of Eq and Hash are correct, you should be able to insert a number into a map by i64 key and look it up by u64 key. Same thing below.

_ => None,
}
}
fn index_into_mut<'v>(&self, v: &'v mut Value) -> Option<&'v mut Value> {
match *v {
Value::Sequence(ref mut vec) => vec.get_mut(*self),
Value::Mapping(ref mut vec) => vec.get_mut(&Value::I64(*self as i64)),
Value::Mapping(ref mut vec) => vec.get_mut(&Value::Number(Number::from(*self as i64))),
_ => None,
}
}
Expand All @@ -61,10 +62,10 @@ impl Index for usize {
Value::Mapping(ref mut map) => {
// TODO: use entry() once LinkedHashMap supports entry()
// https://github.com/contain-rs/linked-hash-map/issues/5
if !map.contains_key(&Value::I64(*self as i64)) {
map.insert(Value::I64(*self as i64), Value::Null);
if !map.contains_key(&Value::Number(Number::from(*self as i64))) {
map.insert(Value::Number(Number::from(*self as i64)), Value::Null);
}
map.get_mut(&Value::I64(*self as i64)).unwrap()
map.get_mut(&Value::Number(Number::from(*self as i64))).unwrap()
},
_ => panic!("cannot access index {} of YAML {}", self, Type(v)),
}
Expand Down Expand Up @@ -165,8 +166,7 @@ impl<'a> fmt::Display for Type<'a> {
match *self.0 {
Value::Null => formatter.write_str("null"),
Value::Bool(_) => formatter.write_str("boolean"),
Value::I64(_) => formatter.write_str("integer"),
Value::F64(_) => formatter.write_str("float"),
Value::Number(_) => formatter.write_str("number"),
Value::String(_) => formatter.write_str("string"),
Value::Sequence(_) => formatter.write_str("sequence"),
Value::Mapping(_) => formatter.write_str("mapping"),
Expand Down