update to nom 5 - Githubissues

Geal commented 5 years ago

reintroduce the cond_reduce combinator that was removed from nom 5

a parser test is failing, but it was already failing before the update, sooooo :D

meh commented 5 years ago

Is it still good practice to keep using the macros with nom 5? The last upgrade to nom 5 I did I converted all the things from macros.

Geal commented 5 years ago

The macros are now a thin layer above the function, so it's using the same code (they force the streaming version of basic parsers). If they work for you, you can keep them for now, and get started on the functions switch later

mitnk commented 5 years ago

I started doing the same thing days ago - upgrade nom to 5 - with noob knowledge on nom, with try-and-fail path. I just wanted to summit a PR before seeing this PR. LOL

So glad to see nom author's solution here :)

I've tried to also fix the failing case mentioned above:

diff --git a/src/parser/source.rs b/src/parser/source.rs
--- a/src/parser/source.rs
+++ b/src/parser/source.rs
@@ -109,7 +112,7 @@ named!(entry<Item>,

                                (Item::String(name, unescape(value))))) >>

-               take_while!(is_ws) >> end >> take_while!(is_eol) >>
+               take_while!(is_ws) >> end >> opt!(complete!(take_while!(is_eol))) >>

But this only make it passed - not sure if it's the right solution.

mitnk commented 5 years ago

Tested this PR with the patch in my last comment - Works fine on Mac and aarch64-linux-android. I think it's good to apply the patch above and enter the CR/merging procedures.

mitnk commented 5 years ago

While, seems other branches also need complete!() when encounter EOF. So the patch should become this:

diff --git a/src/parser/source.rs b/src/parser/source.rs
index 77a6e3c..84fe60d 100644
--- a/src/parser/source.rs
+++ b/src/parser/source.rs
@@ -42,7 +42,7 @@ named!(comment<Item>,
    do_parse!(
        tag!("#") >>
        content: map_res!(terminated!(take_until!("\n"), tag!("\n")), str::from_utf8) >>
-       take_while!(is_eol) >>
+       opt!(complete!(take_while!(is_eol))) >>

        (Item::Comment(content.trim()))));

@@ -58,7 +58,7 @@ named!(definition<Item>,

        tag!(",") >>
        take_while!(is_ws) >>
-       eol >> take_while!(is_eol) >>
+       eol >> opt!(complete!(take_while!(is_eol))) >>

        ({
            let mut aliases = content.split(|c| c == '|').map(|n| n.trim()).collect::<Vec<_>>();
@@ -79,7 +79,7 @@ named!(disable<Item>,
            unsafe { str::from_utf8_unchecked(n) }) >>

        tag!(",") >>
-       take_while!(is_ws) >> end >> take_while!(is_eol) >>
+       take_while!(is_ws) >> end >> opt!(complete!(take_while!(is_eol))) >>

        (Item::Disable(name))));

@@ -109,7 +109,7 @@ named!(entry<Item>,

                (Item::String(name, unescape(value))))) >>

-       take_while!(is_ws) >> end >> take_while!(is_eol) >>
+       take_while!(is_ws) >> end >> opt!(complete!(take_while!(is_eol))) >>

        (value)));

mitnk commented 5 years ago

Hi @meh, we have fixed the CI issue for this PR. Would you like to have check on code change when you got time? If this PR is merged, #12 can be unblocked ;)

Geal commented 4 years ago

friendly ping :wink:

meh commented 4 years ago

Friendly thanks for the ping :panda_face:

meh / rust-terminfo

update to nom 5 #13