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

Support parsing range sets #22

Closed
KodrAus opened this Issue Oct 21, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@KodrAus
Copy link

KodrAus commented Oct 21, 2017

The npm semver package supports parsing sets of versions separated by some operator.

It would be great if semver_parser also supported VersionReq (what npm calls Comparator) ranges where the versions are separated by:

  • || for logical or ranges
  • whitespace for logical and ranges

Are there any others in the npm package we should also support?

I think this will need more discussion if we want to produce mentoring instructions someone can use to implement it. At least we should nail down exactly what we want to support on top of what semver_parser already does.

Mentoring instructions

Leave a comment here on the thread and we'll work out how to get started.

@udoprog

This comment has been minimized.

Copy link
Contributor

udoprog commented Nov 26, 2017

Hey,

I'm playing around with rewriting the parser for semver as a simple lexer + recursive-descent parser here:
https://github.com/udoprog/semver-parser/tree/recursive-descent

The rationale is:

  • Make error reporting more predictable.
  • Can be convinced to report the spans where the error ocurred.
  • More cleanly support these kinds of features without resorting to splitting splits, since the parser ends up being more composable.

The way I'm doing it would (probably) also permit swapping out the parser for an LR(1) parser generated by lalrpop.

Please note that this is only an experiment that I've spent a couple of hours playing around with. I'm at the stage where I'd like to get feedback if this is something desirable for this project or not.

@steveklabnik

This comment has been minimized.

Copy link
Owner

steveklabnik commented Nov 26, 2017

Neat!

What mostly matters to this project is:

  1. correctness
  2. no dependencies
  3. ease of maintenance
  4. errors
  5. performance

I'm not really particularly tied to internals other than those things.

@udoprog

This comment has been minimized.

Copy link
Contributor

udoprog commented Nov 26, 2017

Thanks.

The implementation is now feature complete according to the tests available.

A minor benchmark seems to indicate that it has a slight edge over the current approach, probably because most decisions can be made using a single pass and only one token of lookahead. More importantly there's probably no significant regression (all though this hasn't been rigorously tested):

test range::tests::bench_simple_version        ... bench:         236 ns/iter (+/- 46)
test range_parser::tests::bench_simple_version ... bench:         198 ns/iter (+/- 19)

There are two improvements I'd like to accomplish with the current parser at some point:

  • I've noticed that there is no test coverage for how to deal with whitespace in the middle of version requirements. So the rec-desc parser currently accepts > 0. 1, while the old one doesn't. See: udoprog@b76af35
    I've opened #32 for this.
  • Pre-release identifiers currently require copying for no good reason. I've prepared the infrastructure in the parser I built to do this, but it could also be ported back.
    I've opened #33 for this.
@steveklabnik

This comment has been minimized.

Copy link
Owner

steveklabnik commented Jun 18, 2018

I merged this in, so I think this is good now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.