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

Module hierarchy like in nom? #23

Closed
sdroege opened this issue Jul 8, 2019 · 6 comments
Closed

Module hierarchy like in nom? #23

sdroege opened this issue Jul 8, 2019 · 6 comments

Comments

@sdroege
Copy link
Contributor

sdroege commented Jul 8, 2019

Currently all types are directly at the top-level of the crate. It potentially makes sense to move them into various sub-modules similar to how it's done in nom.

As part of that, the compat std::io module could also be moved into a nicer place.

And maybe we could also move the macros out of the way into some kind of legacy module and mark them as deprecated :)

What do you think?

@Keruspe
Copy link
Collaborator

Keruspe commented Jul 8, 2019

Something definitely needs to be done wrt this, yep

@Keruspe
Copy link
Collaborator

Keruspe commented Jul 8, 2019

cookie_factory::{
    deprecated::macros::*, // meh, macros are always exported at the root of the crate no matter where they are defined but still...
    no_std::io::*,
    *
}

This could be a first step

@sdroege
Copy link
Contributor Author

sdroege commented Jul 8, 2019

Yeah, sounds like a good start to me. I was also thinking about creating modules like nom's branch, combinator, character, sequence, etc. but those modules would look mostly empty right now so maybe not worth it.

@Geal
Copy link
Collaborator

Geal commented Jul 11, 2019

yup, it's a good idea to have modules, better have them now and add stuff later, because changing module paths later will be a breaking change

@sdroege
Copy link
Contributor Author

sdroege commented Jul 11, 2019

Ok, let's go with the top-level modules of nom then as a starting point, plus what @Keruspe said in the first comment above? I can prepare a PR for that if that sounds fine to everybody.

@sdroege
Copy link
Contributor Author

sdroege commented Sep 25, 2019

This is fixed by a265b1c and follow-up commits now

@sdroege sdroege closed this as completed Sep 25, 2019
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

3 participants