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.

Sign up for GitHub

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

(feat) add semver.org example #300

Merged
merged 1 commit into from
Jun 20, 2022
Merged

(feat) add semver.org example #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Balázs Kutil <bkutil@users.noreply.github.com> (https://github.com/bkutil/)
Caleb Hearon <crh0872@gmail.com> (https://github.com/chearon/)
Charles Pick <charles@codemix.com> (https://github.com/phpnode/)
Christian Flach <github@christianflach.de> (https://github.com/cmfcmf/)
Dan Selman <danscode@selman.org> (https://github.com/dselman)
David Berneda <david@steema.com>
Futago-za Ryuu <futagoza.ryuu@gmail.com> (https://github.com/futagoza/)
Jakub Vrana <jakub@vrana.cz> (https://github.com/vrana/)
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ This file documents all notable changes to Peggy.
Unreleased
----------

- [#299](https://github.com/peggyjs/peggy/issues/299) Add example grammar for a
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll fix this up after merge. Apparently I didn't leave a template in the changelog after 2.0.1, because that was done i n such a rush.

[SemVer.org](https://semver.org) semantic version string

Released: TBD

### Major Changes
Expand Down
60 changes: 60 additions & 0 deletions examples/semver.peggy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/**
* SemVer.org v2
* 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 };
Copy link

Choose a reason for hiding this comment

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

Don't think that hiding most important information into a deeper object with non-descript name is a good idea. At least {...versionCore, pre, build}, please.

}

versionCore
= major:$numericIdentifier '.' minor:$numericIdentifier '.' patch:$numericIdentifier
{
return {
major: parseInt(major, 10),
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor parseInt into numericIdentifier.

Copy link
Member

Choose a reason for hiding this comment

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

Moving parseInt to numericIdentifier may be not so good, because that rule is also used in contexts where you do not want a number, but only a match result. I'm fine to leave parsing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

then an intermediate rule that does the parsing, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regardless, the $ should go in numericIdentifier.

minor: parseInt(minor, 10),
patch: parseInt(patch, 10),
};
}

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

@reverofevil reverofevil Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for this. I automatically kept using return tail.unshift(head), tail;.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's in the docs now, look for "Parsing Lists".

}

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

preReleaseIdentifier
= alphanumericIdentifier
/ numericIdentifier

buildIdentifier
= alphanumericIdentifier
/ digit+

alphanumericIdentifier
Copy link

@reverofevil reverofevil Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

According to spec, it should be identifierChar* nonDigit identifierChar*.

If you substitute everything into alphanumericIdentifier, it turns out the only reason why it exists is to avoid parse conflict in preReleaseIdentifier. If there is no nonDigit, it might match same things as numericIdentifier.

In PEG parsers there is no parse conflicts, it can be greatly simplified.

Copy link

@reverofevil reverofevil Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is all you need to comply to the spec:

semver = major:numericId "." minor:numericId "." patch:numericId prerelease:("-" @prerelease)? build:("+" @build)? {
	return {major, minor, patch, prerelease, build};
}
prerelease = head:prereleaseId tail:("." @prereleaseId)* { return [head, ...tail]; }
build = head:alnumId tail:("." @alnumId)* { return [head, ...tail]; }
prereleaseId = alnumId / $("0" / [1-9][0-9]*)
alnumId = $[0-9a-z-]i+

Copy link

@reverofevil reverofevil Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually buildId is just alnumId. It will never hit the second branch. The only reason why grammar in spec distinguishes them, is to stress that numeric identifiers are sorted in numeric order. As this is a parser, and not comparator, it shouldn't really care.

Copy link
Contributor

@MarcelBolten MarcelBolten Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is all you need to comply to the spec:

semver = major:numericId "." minor:numericId "." patch:numericId prerelease:("-" @prerelease)? build:("+" @build)? {
	return {major, minor, patch, prerelease, build};
}
prerelease = head:prereleaseId tail:("." @prereleaseId)* { return [head, ...tail]; }
build = head:alnumId tail:("." @alnumId)* { return [head, ...tail]; }
prereleaseId = alnumId / $("0" / [1-9][0-9]*)
alnumId = $[0-9a-z-]i+

rule numericId is missing, maybe like so:

semver = major:numericId "." minor:numericId "." patch:numericId prerelease:("-" @prerelease)? build:("+" @build)? {
	return {major, minor, patch, prerelease, build};
}
prerelease = head:prereleaseId tail:("." @prereleaseId)* { return [head, ...tail]; }
build = head:alnumId tail:("." @alnumId)* { return [head, ...tail]; }
prereleaseId = alnumId / numericId 
alnumId = $[0-9a-z-]i+
numericId = $("0" / [1-9][0-9]*)

However, while this will successfully parse all valid examples it will also successfully parse invalid examples (1.2.3-0123, 1.2.3-0123.0123) but "numeric identifiers MUST NOT include leading zeroes" in the pre-release part.

= digit* nonDigit identifierChar*

numericIdentifier
= '0' / (positiveDigit digit*)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this?

numericIdentifier
  = num:$(positiveDigit digit*) { return parseInt(num) }
  / '0' ![0-9] { return 0 }

Copy link

Choose a reason for hiding this comment

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

TBH I don't understand how this works right now. positiveDigit digit* should return ['1', ['2', '3']], and parseInt(['1', ['2', '3']], 10) = 1.

Copy link

Choose a reason for hiding this comment

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

I'd suggest a bit different code, though:

numericIdentifier = num:$('0' / [1-9][0-9]*) { return parseInt(num); }

IMO you don't have to write a lot of code to make it more readable. "Positive digit" doesn't add any more information to [1-9].

Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to keep it closer to https://semver.org/spec/v2.0.0.html I think.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't understand how this works right now.

The magic in the $ operator that returns the text from the input instead of results of subexpressions.

"Positive digit" doesn't add any more information to [1-9].

Agreed, especially since this rule is used only once

Copy link

Choose a reason for hiding this comment

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

Oh, I missed $ up above.

Copy link

Choose a reason for hiding this comment

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

Anyway, I didn't find a place in the spec that would limit numbers by any limit, so I suspect parseInt doesn't formally comply to the spec :/


identifierChar
= [a-z0-9-]i

nonDigit
= [a-z-]i

digit
= [0-9]

positiveDigit
= [1-9]
Copy link
Contributor

Choose a reason for hiding this comment

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

newline at end, please

Copy link

Choose a reason for hiding this comment

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

Could you please create an issue to set up husky and some kind of linter, so that nobody has to enforce code style by hand?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a linter for .peggy files yet, as far as I know. There should be one.

We lint all of the JS on each GHA run.