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

Expose the nom parsers in a public parsing:: submodule #82

Merged
merged 2 commits into from
Jan 27, 2017

Conversation

mystor
Copy link
Collaborator

@mystor mystor commented Jan 24, 2017

I quickly threw this together, because this solution is super easy and straightforward.

Fixes #81.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with this approach once it passes in Travis. I am busy with a serde release today but later in the week I will need to go through and write doc comments for all of these macros.

macro_rules! tuple {
($i:expr, $($rest:tt)*) => {
tuple_parser!($i, (), $($rest)*)
};
}

/// Internal parser, do not use directly
#[macro_export]
macro_rules! tuple_parser {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be #[doc(hidden)]

@@ -41,36 +44,39 @@ macro_rules! map {
}

/// Internal parser, do not use directly
#[macro_export]
macro_rules! map_impl {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be #[doc(hidden)]

name = "syn_nom"
version = "0.1.0"
authors = ["David Tolnay <[email protected]>"]
licence = "MIT/Apache-2.0"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cargo wants this to be "license"


pub use lit::parsing::int as int_lit;

pub use lit::parsing::boolean as bool_lit;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have these lit parsers return their own individual types - it seems silly to parse a bool_lit and then have to pattern-match on a Lit to read the bool out of it. There should be a parser called "boolean" that gives you a bool.

For string / byte string / int / float we will need some sort of wrapper struct like struct StrLit(String, StrStyle).

pub use macro_input::parsing::macro_input;

#[cfg(feature = "full")]
pub use krate::parsing::krate;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is one that people will need in their procedural macros, let's leave it out.

//! thus improves build speeds. This should be used instead of `nom` when
//! building parsers using these parsers.
pub use macro_input::parsing::macro_input;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this out, this one is specific to the macros 1.1 custom derive use case and isn't something people will need for other procedural macros.

@@ -10,26 +10,29 @@ pub enum IResult<I, O> {
Error,
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also move all the macros in src/helper.rs - you won't get very far without those, especially punct! and keyword!. Keep them in a separate file though to track that they are not from upstream nom.

@mystor mystor force-pushed the public_nom_parsers branch 3 times, most recently from 947b6b5 to b9b3a81 Compare January 27, 2017 17:27
@mystor mystor force-pushed the public_nom_parsers branch from b9b3a81 to 5e107ff Compare January 27, 2017 17:34
@dtolnay dtolnay merged commit e364b29 into dtolnay:master Jan 27, 2017
@utkarshkukreti
Copy link
Contributor

@dtolnay do you plan to release a new version of syn with these changes soon now that #87 has been fixed? Thanks!

@dtolnay
Copy link
Owner

dtolnay commented Feb 19, 2017

@utkarshkukreti what do you need it for? I was not in a hurry to release because I didn't think this would be useful until function-like procedural macros land in nightly.

@utkarshkukreti
Copy link
Contributor

@dtolnay I'm writing a template engine and need to be able to parse the longest prefix of a str which is a valid expression. I was using a pretty inefficient way to solve this earlier -- trying to parse an expression at every whitespace and taking the last valid one (which was very slow) before I found this pull request.

Thanks for the release!

One quick question: is there any reason you haven't exposed the -> Stmt parsers? Would you accept a PR to make them public? I wouldn't need a release right away -- I'm still at least a week away from doing a public release of my crate.

@dtolnay
Copy link
Owner

dtolnay commented Feb 20, 2017

I agree, perhaps we started with too conservative of a choice of functions. I added stmt as well as pat and block in 1bf1913. Let me know if there are any others that would be useful to you.

@utkarshkukreti
Copy link
Contributor

Thanks, I definitely needed pat as well. I will let you know if I need more!

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

Successfully merging this pull request may close these issues.

None yet

3 participants