nom icon indicating copy to clipboard operation
nom copied to clipboard

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

Open epage opened this issue 4 years ago • 12 comments

Prerequisites

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

  • Rust version : 1.55
  • nom version : 7.0.0
  • nom compilation features used: default

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 avatar Sep 28 '21 14:09 epage

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 avatar Sep 28 '21 14:09 epage

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.

epage avatar Sep 28 '21 14:09 epage

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.

Stargateur avatar Sep 28 '21 14:09 Stargateur

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.

epage avatar Sep 28 '21 15:09 epage

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

Geal avatar Oct 10 '21 20:10 Geal

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

  • 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.

epage avatar Oct 11 '21 14:10 epage

  • 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.

nickelc avatar Oct 11 '21 19:10 nickelc

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.

Stargateur avatar Oct 21 '21 01:10 Stargateur

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

Geal avatar Oct 21 '21 21:10 Geal

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:

  • Wait until 8.0
    • No transition path for users
  • Duplicate paths (now or before 8.0)
    • Old Path options (can be combined):
      • Hide (now or before 8.0)
      • Deprecate (now or before 8.0)
    • nom::characters::*
      • Deprecate (now or before 8,0) in favor of AsChar::as_char and stdlib
      • Deprecate (now or before 8.0) in favor of AsChar::* (we'd need to add AsChar::is_newline and AsChar::is_space)

My original proposal was:

  • Now, duplicate paths
  • Now, hide old paths
  • Now, deprecate old paths
  • Now, deprecate nom::characters::* in favor of AsChar

It sounds like the discussion led to

  • Now, duplicate paths
  • Now, hide old paths
  • Before 8.0, deprecate old paths
  • Before 8.0, deprecate nom::characters::* in favor of AsChar::as_char and stdlib

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

epage avatar Jan 14 '22 18:01 epage

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.

Xiretza avatar Jan 14 '22 19:01 Xiretza

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

epage avatar Dec 13 '22 19:12 epage