Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
epage opened this issue Sep 28, 2021 · 12 comments
Open

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

epage opened this issue Sep 28, 2021 · 12 comments
Milestone

Comments

@epage
Copy link
Contributor

epage commented Sep 28, 2021

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
Copy link
Contributor Author

epage commented Sep 28, 2021

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
Copy link
Contributor Author

epage commented Sep 28, 2021

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
Copy link
Contributor

Stargateur commented Sep 28, 2021

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
Copy link
Contributor Author

epage commented Sep 28, 2021

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
Copy link
Collaborator

Geal commented Oct 10, 2021

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 Geal added this to the 8.0 milestone Oct 10, 2021
@epage
Copy link
Contributor Author

epage commented Oct 11, 2021

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.

@nickelc
Copy link
Contributor

nickelc commented Oct 11, 2021

  • 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
Copy link
Contributor

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
Copy link
Collaborator

Geal commented Oct 21, 2021

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

@epage
Copy link
Contributor Author

epage commented Jan 14, 2022

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?

@Xiretza
Copy link
Contributor

Xiretza commented Jan 14, 2022

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
Copy link
Contributor Author

epage commented Dec 13, 2022

#1535 offers an alternative. See winnow-rs/winnow#28 for what it looks like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants