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

Code review nits for semver grammar #303

Merged
merged 3 commits into from
Jun 22, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@ This file documents all notable changes to Peggy.
Unreleased
----------

- [#299](https://github.com/peggyjs/peggy/issues/299) Add example grammar for a
[SemVer.org](https://semver.org) semantic version string

Released: TBD

### Major Changes
Expand Down Expand Up @@ -37,6 +34,8 @@ Released: TBD
(can be useful for plugin writers), from @Mingun
- [#294](https://github.com/peggyjs/peggy/pull/294) Website: show errors in the
editors, from @Mingun
- [#299](https://github.com/peggyjs/peggy/issues/299) Add example grammar for a
[SemVer.org](https://semver.org) semantic version string, from @dselman

### Bug Fixes

Expand Down
30 changes: 14 additions & 16 deletions examples/semver.peggy
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,27 @@
* https://semver.org/spec/v2.0.0.html
* For unit tests see: https://github.com/dselman/peggy-semver
*/

semver
= versionCore:versionCore
pre:('-' @preRelease)?
build:('+' @build)?
{
return { versionCore, pre, 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),
};
= major:numericIdentifier '.' minor:numericIdentifier '.' patch:numericIdentifier {
return { major, minor, patch };
}

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

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

Expand All @@ -39,13 +33,17 @@ preReleaseIdentifier

buildIdentifier
= alphanumericIdentifier
/ digit+
/ $digit+ // Not a number, buildIdentifiers aren't semantically significant.

// If there is a non-digit anywhere, this label is alphanumeric, and
// is compared lexically. Return a string.
alphanumericIdentifier
= digit* nonDigit identifierChar*
= $(digit* nonDigit identifierChar*)

// Any semantically significant numbers are turned into integers for
// later numeric comparison.
numericIdentifier
= '0' / (positiveDigit digit*)
= n:('0' / $(positiveDigit digit*)) { return parseInt(n, 10); }

Choose a reason for hiding this comment

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

Formally should parse be BigInt(n) instead of parseInt(n, 10).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eh, I guess. We don't have to worry about IE anymore.


identifierChar
= [a-z0-9-]i
Expand All @@ -57,4 +55,4 @@ digit
= [0-9]

positiveDigit
= [1-9]
= [1-9]

Choose a reason for hiding this comment

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

Any other places where [1-9] is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to stay as close as possible to the original grammar.