Skip to content
This repository was archived by the owner on Apr 20, 2020. It is now read-only.

fix #55 return the right type on json.num* call #56

Merged
merged 4 commits into from
Sep 11, 2019
Merged

Conversation

gkorland
Copy link
Contributor

@gkorland gkorland commented Sep 8, 2019

No description provided.

@gkorland gkorland requested a review from gavrie September 8, 2019 14:01
@gavrie gavrie force-pushed the num_incrby branch 2 times, most recently from ccee7ed to 078db8c Compare September 10, 2019 19:08
Copy link
Contributor

@gavrie gavrie left a comment

Choose a reason for hiding this comment

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

OK, so I tried a bunch of different approaches, and finally settled on this one which isn't too different from the original code. It's the most concise with the least duplication. The most likely contender was based on traits, but it turned out much too verbose.

Please see my latest commits, and let's merge if you're happy with it.

gavrie
gavrie previously approved these changes Sep 11, 2019
src/lib.rs Outdated
if let Value::Number(curr_value) = curr_value {
let in_value = &serde_json::from_str(in_value)?;
if let Value::Number(in_value) = in_value {
let (num1, num2) = (curr_value, in_value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do you need this (num1,num2)?

            let num_res = match (curr_value.as_i64(), in_value.as_i64()) {
                (Some(num1), Some(num2)) => op_i64(num1, num2).into(),
                _ => {
                    let num1 = curr_value.as_f64().unwrap();
                    let num2 = in_value.as_f64().unwrap();
                    Number::from_f64(op_f64(num1, num2)).unwrap()
                }
            };

Seems cleaner not to overload the num1,num2 with two types

@gkorland gkorland merged commit 2143ec1 into master Sep 11, 2019
@gkorland gkorland deleted the num_incrby branch September 11, 2019 08:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants