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

peggy.js grammar for semver #299

Closed
dselman opened this issue Jun 16, 2022 · 5 comments
Closed

peggy.js grammar for semver #299

dselman opened this issue Jun 16, 2022 · 5 comments

Comments

@dselman
Copy link
Contributor

dselman commented Jun 16, 2022

I've recently been working on a peggy.js grammar for semver strings. I'd welcome feedback and if suitable you are welcome to add it to your library of example grammars.

https://github.com/dselman/peggy-semver/blob/main/parser.pegjs

@hildjj
Copy link
Contributor

hildjj commented Jun 16, 2022

Interesting. If you'd like to contribute it to the Peggy project, you could remove the Apache license and create a PR where you also add yourself to the AUTHORS file, and a bullet to CHANGELOG.md, which I'd accept.

As far as the grammar goes, here are some suggestions:

  • I don't like the use of text() in your checkNumber function, particularly that far away from the rule. Perhaps pass the text in, and capture it with a label, like: numeric_identifier = n:$[0-9]+ { return checkNumber(n) }
  • Your checkNumber function should be in the global initializer: {{ function... }}.
  • in valid_semver, if you order the rules the other direction, you won't need ! predicates. Same in alphanumeric_identifier
  • In alphanumeric_identifier, there should be a comment about why this looks different than the spec.
  • In positive_digits, you probably just want a string instead of an array. How about: positive_digits = $(positive_digit digit*). Actually, there are a bunch of other places where I think you just want to use $.
  • Perhaps you should return some sort of object like: { major: 1, minor: 0, patch: 0, pre: [ 'alpha' ], build: [ '27' ] }
  • (style nit) I like to orient the main alternatives in a rule like this, as I find it easier to see that the order in which they will be tried
rule
  = alt1
  / alt2
  / alt3

Great work so far!

@MarcelBolten
Copy link
Contributor

MarcelBolten commented Jun 18, 2022

Hi @dselman

I got inspired by your work and tried to avoid the checkNumber function and also included some suggestions of hildjj.
Feel free to take what you like from my attempt for your project.

semver
  = versionCore:versionCore
    pre:('-' @preRelease)?
    build:('+' @build)?
  {
    return { ...versionCore, pre, build };
  }

versionCore
  = major:$numericIdentifier '.' minor:$numericIdentifier '.' patch:$numericIdentifier
  {
    return {
      major: parseInt(major, 10),
      minor: parseInt(minor, 10),
      patch: parseInt(patch, 10),
    };
  }

preRelease
  = head:$preReleaseIdentifier tail:('.' @$preReleaseIdentifier)*
  {
    return [head, ...tail];
  }

build
  = head:$buildIdentifier tail:('.' @$buildIdentifier)*
  {
    return [head, ...tail];
  }

preReleaseIdentifier
  = alphanumericIdentifier
  / numericIdentifier

buildIdentifier
  = alphanumericIdentifier
  / digit+

alphanumericIdentifier
  = digit* nonDigit identifierChar*

numericIdentifier
  = '0'
  / positiveDigit digit*

identifierChar
  = [a-z0-9-]i

nonDigit
  = [a-z-]i

digit
  = [0-9]

positiveDigit
  = [1-9]

@dselman
Copy link
Contributor Author

dselman commented Jun 18, 2022

That's awesome - thank you @hildjj and @MarcelBolten -- much more elegant.

@dselman
Copy link
Contributor Author

dselman commented Jun 18, 2022

PR created: #300

@hildjj
Copy link
Contributor

hildjj commented Jun 30, 2022

Fixed in #304

@hildjj hildjj closed this as completed Jun 30, 2022
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