Json stringify replacer

[n14little]

Closes #345
Fixes #403

Additionally, wrote and ignored some tests that should be addressed once #405 is fixed.

[Razican]

This is looking pretty good for me. I like the idea of creating a new object if needed, but I would add a fast path in the case that args 1 and 2 are not passed, just returning the JSON of the provided object, it should improve performance in most cases.

[n14little]

@Razican --- The error on the test in commit b22e5b9 feels like a bug, would you agree?

[n14little]

Once I get the symbol thing figured out, I should be able to wrap this up pretty quickly.

[Razican]

@Razican --- The error on the test in commit b22e5b9 feels like a bug, would you agree?

Hmm this depends. Does the test panic if you run it against master? if so, we should open an issue about it.

[n14little]

@Razican -- Yes, it happens on master. Just opened an issue #405

[Razican]

@Razican -- Yes, it happens on master. Just opened an issue #405

Good, so comment that out and we can progress with this issue :)

[Razican]

I checked the spec and thought of an arguably simpler design:

pub fn stringify(_: &mut Value, args: &[Value], interpreter: &mut Interpreter) -> ResultValue {
    let object = args.get(0).expect("cannot get argument for JSON.stringify");

    if args.len() == 1 {
        return Ok(Value::from(object.to_json().to_string()));
    }

    let replacer = args.get(1).expect("first element disappeared");
    if !replacer.is_object() {
        return Ok(Value::from(object.to_json().to_string()));
    }

    let replacer = replacer.as_object().expect("replacer was an object");
    if replacer.is_callable() {
        unimplemented!("replacer function");
    } else if replacer.kind == ObjectKind::Array {
        let mut obj_to_return = serde_json::Map::with_capacity(replacer.properties.len() - 1);
        for field in replacer.properties.iter().filter_map(|(key, prop)| {
            if key == "length" {
                None
            } else {
                // TODO: this should be the abstract operation `Get`
                prop.value.as_ref().map(Value::to_string)
            }
        }) {
            if let Some(value) = object
                .get_property(&field)
                .map(|prop| prop.value.as_ref().map(|v| v.to_json()))
                .flatten()
            {
                obj_to_return.insert(field, value);
            }
        }
        Ok(Value::from(JSONValue::Object(obj_to_return).to_string()))
    } else {
        Ok(Value::from(object.to_json().to_string()))
    }
}

Note that I didn't implement the function replacer and that I repeated Ok(Value::from(object.to_json().to_string())) too many times, so this should probably be replaced by a function or something.

The idea behind this is to avoid all that nesting. Then, instead of directly looping through all properties, I filtered the property iterator and mapped it to just return the string values.

Then, I just start creating a JSONValue, since there is no need to create an object to then convert it to JSON. I add only properties that exist and have a value, not sure if this this is 100% spec compliant, I just skimmed through it.

I did a bit of a heavy use of the Option API there, I recommend you to check it. Let me know if there is something unclear :)

[n14little]

@Razican -- thanks for taking the time to write something up. I've implemented the array replacer functionality per your suggestion. Still working on the function replacer.

The idea behind this is to avoid all that nesting.

That is what I was hoping to get rid of.

[n14little]

Made an attempt at using Options in the function replacer branch.

[n14little]

@Razican thoughts on how this is looking?

[Razican]

@Razican thoughts on how this is looking?

I will check this today :)

[Razican]

I added some comments, give them a look :)

[n14little]

@Razican --- addressed all of your comments.

[Razican]

Looks very good! almost ready, I just added a couple of comments, one to improve a bit how we call the function and the other one to take advantage of a change that was just merged to master :)

[Razican]

This is good to go for me :) we should at some point review how we write tests, they are a bit too wordy, and I think we can improve them, but it's not directly related to this PR.

[HalidOdat]

Looks good! check my comments to see on how we can improve it :)

[n14little]

@HalidOdat -- addressed your comments. Added a test for the case where nothing is passed to stringify.

[Razican]

Looks very good, check my comments to improve readability a bit :)

[n14little]

@Razican @HalidOdat -- I think that I have all comments addressed.

[Razican]

Looks good to me! :)