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

Update usage parser (potentially with nom or chomp) #350

Closed
kbknapp opened this issue Nov 24, 2015 · 4 comments
Closed

Update usage parser (potentially with nom or chomp) #350

kbknapp opened this issue Nov 24, 2015 · 4 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations
Milestone

Comments

@kbknapp
Copy link
Member

kbknapp commented Nov 24, 2015

With nom recently reaching 1.0 I'm wondering if using would be any benefit to this project. Particularly in the test-ability, performance, and maintainability.

Because I'm very unfamiliar with it, but slowly reading through it, this may be slow going.

Perhaps initially evaluating on the from_usage parser.

To be clear, in order to be merged into mainline it needs to not impact performance at all (which I don't believe will be an issue), and must improve at least one of the items above.

@kbknapp kbknapp added C-enhancement Category: Raise on the bar on expectations P4: nice to have A-parsing Area: Parser's logic and needs it changed somehow. labels Nov 24, 2015
@Vinatorul
Copy link
Contributor

From the one side - we can rewrite parser no nom and make it more clear.
From the other - nom is new dependency.

As for me - I'm pretty much for rewriting :)

@kbknapp
Copy link
Member Author

kbknapp commented Nov 25, 2015

Yeah I understand the new dep thing, but if we can make the code more readable, concise, testable, and performant then I'm all ok using a new dep. If we can do all the above by just re-writing the parser I'm good with that too. But I would imagine handwriting a new parser might be re-implenting things already solved innom

Also chomp looks promising too but the fact that it's less established and pre-1.0 means I'd lean towards nom all things being equal.

To be clear, I'm not really advocating using nom in master yet, more just testing to see if it's something we should start using.

@kbknapp
Copy link
Member Author

kbknapp commented Nov 25, 2015

In all honesty using an iterator in the usage parser is silly, but I guess it made sense to me when I did it. We'd probably get way better performance using a real parser, which is what I'd really like to explore. Whether that's using something like nom or home rolled? Let's try them both and see what looks/performs best! 😄

@kbknapp kbknapp added this to the v2.0 milestone Jan 23, 2016
@kbknapp kbknapp added W: 2.x and removed W: 1.x labels Jan 25, 2016
@kbknapp kbknapp changed the title Evaluate potential for use of nom Update usage parser (potentially with nom or chomp) Jan 25, 2016
kbknapp added a commit that referenced this issue Jan 25, 2016
@kbknapp
Copy link
Member Author

kbknapp commented Jan 26, 2016

Closed with #386

@kbknapp kbknapp closed this as completed Jan 26, 2016
kbknapp added a commit that referenced this issue Jan 28, 2016
kbknapp added a commit that referenced this issue Jan 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants