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 into_ counterparts for all as_ methods #28

Closed
matthew-piziak opened this issue Aug 7, 2016 · 3 comments · Fixed by #29
Closed

Add into_ counterparts for all as_ methods #28

matthew-piziak opened this issue Aug 7, 2016 · 3 comments · Fixed by #29

Comments

@matthew-piziak
Copy link
Contributor

Background

Conversion method naming conventions

RFC 7087 and The Rust Style Guidelines define the following naming conventions for conversion methods:

Prefix Cost Consumes convertee
as_ Free No
to_ Expensive No
into_ Variable Yes

Proposed Enhancement

Currently yaml-rust provides as_ methods (as_str, as_hash, as_vec, etc.) on the impl Yaml. These methods obey the listed convention and do not consume the Yaml object. Adding into_ methods that consume the Yaml object would allow some callers to reduce their overall memory footprint.

Use Case

Code Review Answer (last paragraph)

@matthew-piziak
Copy link
Contributor Author

I plan to submit a pull request for this ticket in the next few days.

@matthew-piziak
Copy link
Contributor Author

Here's what I currently have:

Macro and Methods

macro_rules! define_as (
    ($name:ident, $t:ident, $yt:ident) => (
pub fn $name(&self) -> Option<$t> {
    match *self {
        Yaml::$yt(v) => Some(v),
        _ => None
    }
}
    );
);

macro_rules! define_as_ref (
    ($name:ident, $t:ty, $yt:ident) => (
pub fn $name(&self) -> Option<$t> {
    match *self {
        Yaml::$yt(ref v) => Some(v),
        _ => None
    }
}
    );
);

macro_rules! define_into (
    ($name:ident, $t:ty, $yt:ident) => (
pub fn $name(self) -> Option<$t> {
    match self {
        Yaml::$yt(v) => Some(v),
        _ => None
    }
}
    );
); 

impl Yaml {
    define_as!(as_bool, bool, Boolean);
    define_as!(as_i64, i64, Integer);

    define_as_ref!(as_str, &str, String);
    define_as_ref!(as_hash, &Hash, Hash);
    define_as_ref!(as_vec, &Array, Array);

    define_into!(into_bool, bool, Boolean);
    define_into!(into_i64, i64, Integer); 
    define_into!(into_string, String, String);
    define_into!(into_hash, Hash, Hash);
    define_into!(into_vec, Array, Array);

    pub fn is_null(&self) -> bool {
        match *self {
            Yaml::Null => true,
            _ => false
        }
    }

    pub fn is_badvalue(&self) -> bool {
        match *self {
            Yaml::BadValue => true,
            _ => false
        }
    }

    pub fn as_f64(&self) -> Option<f64> {
        match *self {
            Yaml::Real(ref v) => {
                v.parse::<f64>().ok()
            },
            _ => None
        }
    }

    pub fn into_f64(self) -> Option<f64> {
        match self {
            Yaml::Real(v) => {
                v.parse::<f64>().ok()
            },
            _ => None
        }
    }
}

Naïve test case

I'd like to avoid clone but I'm new to Rust and I don't know what the correct improvement is. I would iterate over Yaml to consume the elements directly but although Yaml supports indexing, it does not implement IntoIterator. Is implementing IntoIterator a reasonable choice?

    #[test]
    fn test_plain_datatype_with_into_methods() {
        let s =
"
- 'string'
- \"string\"
- string
- 123
- -321
- 1.23
- -1e4
- true
- false
- !!str 0
- !!int 100
- !!float 2
- !!bool true
- !!bool false
- 0xFF 
- 0o77
- [ 0xF, 0xF ]
- +12345
- [ true, false ]
";
        let out = YamlLoader::load_from_str(&s).unwrap();
        let doc = &out[0]; 

        assert_eq!(doc[0].clone().into_string().unwrap(), "string");
        assert_eq!(doc[1].clone().into_string().unwrap(), "string");
        assert_eq!(doc[2].clone().into_string().unwrap(), "string");
        assert_eq!(doc[3].clone().into_i64().unwrap(), 123);
        assert_eq!(doc[4].clone().into_i64().unwrap(), -321);
        assert_eq!(doc[5].clone().into_f64().unwrap(), 1.23);
        assert_eq!(doc[6].clone().into_f64().unwrap(), -1e4);
        assert_eq!(doc[7].clone().into_bool().unwrap(), true);
        assert_eq!(doc[8].clone().into_bool().unwrap(), false);
        assert_eq!(doc[9].clone().into_string().unwrap(), "0");
        assert_eq!(doc[10].clone().into_i64().unwrap(), 100);
        assert_eq!(doc[11].clone().into_f64().unwrap(), 2.0);
        assert_eq!(doc[12].clone().into_bool().unwrap(), true);
        assert_eq!(doc[13].clone().into_bool().unwrap(), false);
        assert_eq!(doc[14].clone().into_i64().unwrap(), 255);
        assert_eq!(doc[15].clone().into_i64().unwrap(), 63);
        assert_eq!(doc[16][0].clone().into_i64().unwrap(), 15);
        assert_eq!(doc[16][1].clone().into_i64().unwrap(), 15);
        assert_eq!(doc[17].clone().into_i64().unwrap(), 12345);
        assert!(doc[18][0].clone().into_bool().unwrap());
        assert!(!doc[18][1].clone().into_bool().unwrap());
    }

@matthew-piziak
Copy link
Contributor Author

I've implemented IntoIterator and removed the calls to clone.

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 a pull request may close this issue.

1 participant