rust-bakery / nom

Rust parser combinator framework
MIT License
9.45k stars 804 forks source link

Swap module hierarchy so `streaming`/`complete` is in the root #1414

Open epage opened 3 years ago

epage commented 3 years ago

Prerequisites

Here are a few things you should provide to help me understand the issue:

Test case

Please provide a short, complete (with crate import, etc) test case for the issue, showing clearly the expected and obtained results.

Example test case:

use nom::{
    branch, bytes::complete as bytes, character::complete as character, combinator, multi,
    sequence, AsChar, IResult as Outcome,
};

pub(crate) fn dec_int(input: &str) -> IResult<&str, &str> {
    combinator::recognize(sequence::tuple((
        combinator::opt(multi::alt((character::char('+'), character::char('-')))),
        multi::alt((
            character::char('0'),
            combinator::map(
                sequence::tuple((
                    character::satisfy(|c| ('1'..='9').contains(&c)),
                    multi::take_while(is_dec_digit_with_sep),
                )),
                |t| t.0,
            ),
        )),
    )))(input)
}

@Stargateur said

I could see a world where we inverse foo::complete and foo::streaming for complete::foo and streaming::foo this make sense cause:

* I don't think there is a case where in the same parser you mix complete and streaming

* This allow to reduce the number of page in doc

* This remove the need to write `bytes::complete as bytes`

* Would allow people to import every `complete` or `streaming` modules in one line `complete::*`

* Allow to more easily import module by reducing the fragmentation `complete::{foo, bar, baz}` vs `{foo::complete as foo, bar::complete as bar, baz::complete as baz`

Forked from #1408

epage commented 3 years ago

We can do this in a way so that we don't immediately break compatibility. We can move to the new module paths and provide stub modules that pull everything in and just not list them in the docs.

The big question is what to do about the items in nom::character::* and nom::number::Endianess

epage commented 3 years ago

Whats odd is nom::character::* has functions that operate on bytes.

AsChar almost provides everything in that module. Its just missing is_space and is_newline. I wonder if we should add those to AsChar and deprecate the functions. Pointing to AsChar will make it so people find the version of functions that work with both bytes and str.

Stargateur commented 3 years ago

AsChar almost provides everything in that module. Its just missing is_space and is_newline. I wonder if we should add those to AsChar and deprecate the functions. Pointing to AsChar will make it so people find the version of functions that work with both bytes and str.

I work on a crate that contains ABNF core rules, still not published for the upgrade but its use AsChar.

Since most nom::character::* function are just a duplicate of what can be find in core for example, is_ascii_alphabetic() I wonder if we could just remove them at some point.

For now we could just let them stay where there are.

We can do this in a way so that we don't immediately break compatibility. We can move to the new module paths and provide stub modules that pull everything in and just not list them in the docs.

meh, I think this could just add confusion maybe just do it for nom 8, that just a move, that not as we get rid of a feature.

Endianess can just go a top level.

epage commented 3 years ago

I wonder if we could just remove them at some point.

meh, I think this could just add confusion maybe just do it for nom 8, that just a move, that not as we get rid of a feature.

With clap and other crates, I've found its nice to provide a transition path for people, especially if you can mark something as deprecated so people get a message in-line to their workflow telling them how to transition.

I would think a module hidden in the docs that gives people a deprecation message to use the other module path wouldn't be confusing. New users would only see the new version. Existing users would have the deprection message clarifying whats going on.

Geal commented 3 years ago

having top level streaming and complete modules would make sense. I'm not particularly worried about the character module elements, they could be exported through multiple paths. That'll have to wait for nom 8 though

epage commented 3 years ago

That'll have to wait for nom 8 though

Why does this need to wait for 8.0? We could provide both paths now, with deprecation messages

nickelc commented 3 years ago
  • This let's us do it now

  • This helps people through breaking changes in 8.0 since the compiler is telling them how to fix them, rather than reading a CHANGELOG.

nom 7.0 was just released 7 weeks ago that removed macros. I would say that some time should pass before you throw deprecation warnings at people.

Stargateur commented 3 years ago

Why does this need to wait for 8.0? We could provide both paths now, with deprecation messages

* This let's us do it now

* This helps people through breaking changes in 8.0 since the compiler is telling them how to fix them, rather than reading a CHANGELOG.

As I said, more clarity, having duplicate path is not helping. Better do it in one time. For example, std have this problem https://doc.rust-lang.org/std/?search=usize.

Geal commented 3 years ago

we can do that without adding the deprecation warnings. Those can come in a minor version before 8.0

epage commented 2 years ago

So we have many options here, I just want to confirm what is acceptable for moving forward before I implement.

Quick summary of everything on the table so far:

My original proposal was:

It sounds like the discussion led to

Is that correct and we ready to move forward with it?

Xiretza commented 2 years ago

I feel like hiding the old paths without adding deprecation warnings will lead to confusion. Code will compile silently, but when trying to understand what it does, the nom docs will be all wrong unless you figure out you have to select the correct minor version. Maybe that's just me though.

epage commented 1 year ago

1535 offers an alternative. See epage/nom-experimental#28 for what it looks like