Skip to content

Use indexmap to preserve order (optional) #172

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

Merged
merged 5 commits into from
Jun 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@ branches:
- staging
- trying
- master

script:
- cargo test
- cargo test --all-features
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ name = "ron"
[dependencies]
base64 = "0.10"
bitflags = "1"
indexmap = { version = "1.0.2", features = ["serde-1"], optional = true }
serde = { version = "1", features = ["serde_derive"] }

[dev-dependencies]
Expand Down
6 changes: 3 additions & 3 deletions src/de/value.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{collections::BTreeMap, fmt};
use std::fmt;

use serde::{
de::{Error, MapAccess, SeqAccess, Visitor},
Expand All @@ -7,7 +7,7 @@ use serde::{

use crate::{
de,
value::{Number, Value},
value::{Map, Number, Value},
};

impl Value {
Expand Down Expand Up @@ -153,7 +153,7 @@ impl<'de> Visitor<'de> for ValueVisitor {
where
A: MapAccess<'de>,
{
let mut res: BTreeMap<Value, Value> = BTreeMap::new();
let mut res: Map = Map::new();

while let Some(entry) = map.next_entry()? {
res.insert(entry.0, entry.1);
Expand Down
155 changes: 137 additions & 18 deletions src/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,132 @@ use serde::{
DeserializeOwned, DeserializeSeed, Deserializer, Error as SerdeError, MapAccess, SeqAccess,
Visitor,
},
forward_to_deserialize_any,
forward_to_deserialize_any, Deserialize, Serialize,
};
use std::{
cmp::{Eq, Ordering},
collections::BTreeMap,
hash::{Hash, Hasher},
iter::FromIterator,
ops::{Index, IndexMut},
};

use crate::de::{Error as RonError, Result};

/// A `Value` to `Value` map.
///
/// This structure either uses a [BTreeMap](std::collections::BTreeMap) or the
/// [IndexMap](indexmap::IndexMap) internally.
/// The latter can be used by enabling the `indexmap` feature. This can be used
/// to preserve the order of the parsed map.
#[derive(Clone, Debug, Default, Deserialize, Serialize)]
pub struct Map(MapInner);

impl Map {
/// Creates a new, empty `Map`.
pub fn new() -> Map {
Default::default()
}

/// Returns the number of elements in the map.
pub fn len(&self) -> usize {
self.0.len()
}

/// Returns `true` if `self.len() == 0`, `false` otherwise.
pub fn is_empty(&self) -> usize {
self.0.len()
}

/// Inserts a new element, returning the previous element with this `key` if
/// there was any.
pub fn insert(&mut self, key: Value, value: Value) -> Option<Value> {
self.0.insert(key, value)
}

/// Removes an element by its `key`.
pub fn remove(&mut self, key: &Value) -> Option<Value> {
self.0.remove(key)
}

/// Iterate all key-value pairs.
pub fn iter(&self) -> impl Iterator<Item = (&Value, &Value)> + DoubleEndedIterator {
self.0.iter()
}

/// Iterate all key-value pairs mutably.
pub fn iter_mut(&mut self) -> impl Iterator<Item = (&Value, &mut Value)> + DoubleEndedIterator {
self.0.iter_mut()
}

/// Iterate all keys.
pub fn keys(&self) -> impl Iterator<Item = &Value> + DoubleEndedIterator {
self.0.keys()
}

/// Iterate all values.
pub fn values(&self) -> impl Iterator<Item = &Value> + DoubleEndedIterator {
self.0.values()
}

/// Iterate all values mutably.
pub fn values_mut(&mut self) -> impl Iterator<Item = &mut Value> + DoubleEndedIterator {
self.0.values_mut()
}
}

impl FromIterator<(Value, Value)> for Map {
fn from_iter<T: IntoIterator<Item = (Value, Value)>>(iter: T) -> Self {
Map(MapInner::from_iter(iter))
}
}

/// Note: equality is only given if both values and order of values match
impl Eq for Map {}

impl Hash for Map {
fn hash<H: Hasher>(&self, state: &mut H) {
self.iter().for_each(|x| x.hash(state));
}
}

impl Index<&Value> for Map {
type Output = Value;

fn index(&self, index: &Value) -> &Self::Output {
&self.0[index]
}
}

impl IndexMut<&Value> for Map {
fn index_mut(&mut self, index: &Value) -> &mut Self::Output {
self.0.get_mut(index).expect("no entry found for key")
}
}

impl Ord for Map {
fn cmp(&self, other: &Map) -> Ordering {
self.iter().cmp(other.iter())
}
}

/// Note: equality is only given if both values and order of values match
impl PartialEq for Map {
fn eq(&self, other: &Map) -> bool {
self.iter().zip(other.iter()).all(|(a, b)| a == b)
}
}

impl PartialOrd for Map {
fn partial_cmp(&self, other: &Map) -> Option<Ordering> {
self.iter().partial_cmp(other.iter())
}
}

#[cfg(not(feature = "indexmap"))]
type MapInner = std::collections::BTreeMap<Value, Value>;
#[cfg(feature = "indexmap")]
type MapInner = indexmap::IndexMap<Value, Value>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if we just typedef type Map = indexmap::IndexMap<..> instead of wrapping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll end up with a non-additive feature. BTreeMap and IndexMap have different properties (traits, methods, etc.).

Also, APIs accepting BTreeMap would break once indexmap is enabled.


/// A wrapper for `f64` which guarantees that the inner value
/// is finite and thus implements `Eq`, `Hash` and `Ord`.
#[derive(Copy, Clone, Debug)]
Expand All @@ -32,10 +148,10 @@ impl Number {
}

/// Partial equality comparison
/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
/// are equals to each other. It is not the case for underlying `f64` values itself.
/// In order to be able to use `Number` as a mapping key, NaN floating values
/// wrapped in `Number` are equals to each other. It is not the case for
/// underlying `f64` values itself.
impl PartialEq for Number {

fn eq(&self, other: &Self) -> bool {
if self.0.is_nan() && other.0.is_nan() {
return true;
Expand All @@ -45,8 +161,9 @@ impl PartialEq for Number {
}

/// Equality comparison
/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
/// are equals to each other. It is not the case for underlying `f64` values itself.
/// In order to be able to use `Number` as a mapping key, NaN floating values
/// wrapped in `Number` are equals to each other. It is not the case for
/// underlying `f64` values itself.
impl Eq for Number {}

impl Hash for Number {
Expand All @@ -56,9 +173,9 @@ impl Hash for Number {
}

/// Partial ordering comparison
/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
/// are equals to each other and are less then any other floating value.
/// It is not the case for underlying `f64` values itself.
/// In order to be able to use `Number` as a mapping key, NaN floating values
/// wrapped in `Number` are equals to each other and are less then any other
/// floating value. It is not the case for underlying `f64` values itself.
/// ```
/// use ron::value::Number;
/// assert!(Number::new(std::f64::NAN) < Number::new(std::f64::NEG_INFINITY));
Expand All @@ -70,15 +187,16 @@ impl PartialOrd for Number {
(true, true) => Some(Ordering::Equal),
(true, false) => Some(Ordering::Less),
(false, true) => Some(Ordering::Greater),
_ => self.0.partial_cmp(&other.0)
_ => self.0.partial_cmp(&other.0),
}
}
}

/// Ordering comparison
/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
/// are equals to each other and are less then any other floating value.
/// It is not the case for underlying `f64` values itself. See the `PartialEq` implementation.
/// In order to be able to use `Number` as a mapping key, NaN floating values
/// wrapped in `Number` are equals to each other and are less then any other
/// floating value. It is not the case for underlying `f64` values itself. See
/// the `PartialEq` implementation.
impl Ord for Number {
fn cmp(&self, other: &Self) -> Ordering {
self.partial_cmp(other).expect("Bug: Contract violation")
Expand All @@ -89,7 +207,7 @@ impl Ord for Number {
pub enum Value {
Bool(bool),
Char(char),
Map(BTreeMap<Value, Value>),
Map(Map),
Number(Number),
Option(Option<Box<Value>>),
String(String),
Expand Down Expand Up @@ -125,7 +243,7 @@ impl<'de> Deserializer<'de> for Value {
match self {
Value::Bool(b) => visitor.visit_bool(b),
Value::Char(c) => visitor.visit_char(c),
Value::Map(m) => visitor.visit_map(Map {
Value::Map(m) => visitor.visit_map(MapAccessor {
keys: m.keys().cloned().rev().collect(),
values: m.values().cloned().rev().collect(),
}),
Expand Down Expand Up @@ -204,12 +322,12 @@ impl<'de> Deserializer<'de> for Value {
}
}

struct Map {
struct MapAccessor {
keys: Vec<Value>,
values: Vec<Value>,
}

impl<'de> MapAccess<'de> for Map {
impl<'de> MapAccess<'de> for MapAccessor {
type Error = RonError;

fn next_key_seed<K>(&mut self, seed: K) -> Result<Option<K::Value>>
Expand Down Expand Up @@ -257,6 +375,7 @@ mod tests {
use super::*;
use serde::Deserialize;
use std::fmt::Debug;
use std::collections::BTreeMap;

fn assert_same<'de, T>(s: &'de str)
where
Expand Down
76 changes: 76 additions & 0 deletions tests/129_indexmap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#[cfg(feature = "indexmap")]
use ron::{de::from_str, Value};

#[test]
#[cfg(feature = "indexmap")]
fn test_order_preserved() {
let file = r#"(
tasks: {
"debug message": Dbg(
msg: "test message. some text after it."
),
"shell command": Shell(
command: "ls",
args: Some([
"-l",
"-h",
]),
ch_dir: Some("/")
),
}
)
"#;

let value: Value = from_str(file).unwrap();
match value {
Value::Map(map) => match &map[&Value::String("tasks".to_owned())] {
Value::Map(map) => {
assert_eq!(
*map.keys().next().unwrap(),
Value::String("debug message".to_string())
);
assert_eq!(
*map.keys().skip(1).next().unwrap(),
Value::String("shell command".to_string())
);
}
_ => panic!(),
},
_ => panic!(),
}

let file = r#"(
tasks: {
"shell command": Shell(
command: "ls",
args: Some([
"-l",
"-h",
]),
ch_dir: Some("/")
),
"debug message": Dbg(
msg: "test message. some text after it."
),
}
)
"#;

let value: Value = from_str(file).unwrap();
match value {
Value::Map(map) => match &map[&Value::String("tasks".to_owned())] {
Value::Map(map) => {
assert_eq!(
*map.keys().next().unwrap(),
Value::String("shell command".to_string())
);
assert_eq!(
*map.keys().skip(1).next().unwrap(),
Value::String("debug message".to_string())
);
}
_ => panic!(),
},
_ => panic!(),
}
}
5 changes: 2 additions & 3 deletions tests/value.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use ron::value::{Number, Value};
use ron::value::{Map, Number, Value};
use serde::Serialize;
use std::collections::BTreeMap;

#[test]
fn bool() {
Expand All @@ -15,7 +14,7 @@ fn char() {

#[test]
fn map() {
let mut map = BTreeMap::new();
let mut map = Map::new();
map.insert(Value::Char('a'), Value::Number(Number::new(1f64)));
map.insert(Value::Char('b'), Value::Number(Number::new(2f64)));
assert_eq!(Value::from_str("{ 'a': 1, 'b': 2 }"), Ok(Value::Map(map)));
Expand Down