parse negative int, with reader unit tests

[erkkikeranen]

Parses negative integers #28
Adds unit tests to reader.rs #7

(def a -1)
(println (+ a -1))
; returns -2

[Tko1]

Hey so you've figured out the first part; integer_parser was panicking trying to parse strings like - and -211-, blowing up the program instead of returning a soft 'Err' to indicate an error and to give the parser the information it needs to make a decision (without halting the program), such as in try_reader when it will use this information to move on to the next try-reader. Sort of like when people in bad relationships blow up on eachother instead of just letting the other know there's a problem so they can make an adjustment what am I talking about) . The next part we want to do is replace your hand written parsing (ie, the Some('-') .... and all that) with a nom parser, and to also use nom to combine this with the other named!(integer_lexer<&str, &str>, take_while1!(|c: char| c.is_digit(10))); parser (I can do it or you can do it if you like).

If you end up doing it, it'll probably be good to lookup the opt parser if you're unfamiliar with it. I'm gonna try to write more tomorrow, such as an example combining the results of that opt parser with the results of the other parser, I gotta get to sleep, I'm moving currently so I'm busy for a little bit, hope this helps! Thanks so much for messing with this

[Tko1]

So I don't know if I missed any changes but the error I saw when this was initially posted (and this appears to be the same change) was

named!(integer_head<&str, char>,
       map!(
           take_while_m_n!(1, 1, |c: char| is_minus_char(c) || c.is_digit(10)),
           first_char
       )
    );

This is basically saying "A number is

1. A minus or a number - required
2. Maybe more numbers".
   The problem then is - is considered a valid number,

We want something close to the opposite. This is completely opposite, but won't work either:

1. Maybe we have a number or minus
2. Then we have more numbers (required)"

because it won't allow single digit, positive numbers (ie, 1, 2, 3 ... 9) , as they will count as satisfying "1. Maybe we have a number or minus", and then nom will be expecting more numbers to satisfy # 2, which is required (unless nom is somehow more clever than I'm aware; I might test this one to be sure)

So, what we want is

1. Maybe we have a - (instead of 'We have a '-' or digit)
2. Then, we definitely have 1 or more numbers (instead of 'We may have some numbers')

So with

named!(integer_head<&str, char>,
       map!(
           take_while_m_n!(1, 1, |c: char| is_minus_char(c) || c.is_digit(10)),
           first_char
       )
    );

to turn this, which requires 1 match, into a "maybe 1 match", we need to wrap it in opt!, something like

named!(integer_head<&str, char>,
       map!(
           opt!(take_while_m_n!(1, 1, |c: char| is_minus_char(c))),
           // Have to use a different function here, because ^ will return an Option<&str> now 
       )
    );

And instead of this

    named!(integer_tail<&str, &str>, take_while!(|c: char| c.is_digit(10)));

which will 'maybe' return digits (ie, 0 or more), we will write

    // Must return some digits -- 1 or more 
    named!(integer_tail<&str, &str>, take_while1!(|c: char| c.is_digit(10)));

Or, altogether, something like

pub fn integer_parser(input: &str) -> IResult<&str, i32> {
    // Now we return a string instead,  which is "" if there's no -, and "-" if there is 
    named!(integer_head<&str, &str>,
       map!(
           // This parsing is optional, so the overall parse won't fail if there's no "-"
           // It will instead return Some("-") if it finds "-", None otherwise 
           opt!(take_while_m_n!(1, 1, |c: char| is_minus_char(c))),
           // We will the convert that result to "-" or "" 
           |maybe_minus| maybe_minus.unwrap_or("")
       )
    );
    named!(integer_tail<&str, &str>, take_while1!(|c: char| c.is_digit(10)));
    named!(integer_lexer <&str, String>,
         do_parse!(
             head: integer_head >>
             rest_input: integer_tail >>
             // Now we concat *both* as strings
             (format!("{}{}",head,rest_input))
         )
    );
    integer_lexer(input).map(|(rest, digits)| (rest, digits.parse().unwrap()))
}

And note that's likewise just the quick and dirty attempt, showing exactly what changes, you may write the same thing in a prettier way

[scrabsha]

This has been discussed on the Discord server earlier. I proposed either the fix you detailed, or an other on, a bit derpy, but valid in my opinion.

You can also try to see if input starts with -. In such case, you can save its presence in a boolean, remove it from the input, parse the next digits, and when you are about to return the integer, remember to multiply it by -1

In my opinion, the design you described in more idiomatic and describes the grammar better. However, both design should generate correct results.

[Tko1]

Indeed I did not see that, although that's ok; I want to always have an answer on here anyways so there's something attached officially to the repo and issue, and as a record

[Tko1]

Last change; I assume this function was part of another attempt? This can be safely removed I assume, everything else looks great!

[erkkikeranen]

good point! removed and squished

[Tko1]

Awesome!!