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

Formalize grammar #22

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Formalize grammar #22

merged 1 commit into from
Nov 30, 2023

Conversation

frostburn
Copy link
Member

Replace the parser with a PEG.js AST parser and an AST evaluator.

ref #21

@frostburn
Copy link
Member Author

Tests pass over at scale-workshop too if this branch is used as the dependency.

src/sw2.pegjs Outdated
_ = Whitespace*

Expression
= head:Term tail:(_ ('+' / '-') _ Term)* {

This comment was marked as resolved.

@frostburn

This comment was marked as resolved.

src/sw2.pegjs Outdated
= $([+-]? (SlashFraction / PlainNumber))

Monzo
= '[' components:(_ @Component _ ','? _)* '>' { return Monzo(components) }

This comment was marked as resolved.

Replace the parser with a PEG.js AST parser and an AST evaluator.

ref #21
@frostburn
Copy link
Member Author

Ran this by some scales from the Xenharmonic Alliance Discord. Seems to produce the same results.

@arseniiv
Copy link

arseniiv commented Nov 28, 2023

I see signed integers fractions and comma-decimals should be accepted by the parser; it’s probably hard to check if the whole expression represents something well-defined at parse time but maybe this wasn’t intended.

SW2 doesn’t accept the scale or/and ignores those entries. Even in SW2, -3/2 and -1,5 might have been understood as 2/3 as this additive arithmetic is multiplicative on ratios, but I get why it could’ve been overlooked or intentionally left undefined, as it’s doubly confusing.

@frostburn
Copy link
Member Author

I see signed integers fractions and comma-decimals should be accepted by the parser; it’s probably hard to check if the whole expression represents something well-defined at parse time but maybe this wasn’t intended.

SW2 doesn’t accept the scale or/and ignores those entries. Even in SW2, -3/2 and -1,5 might have been understood as 2/3 as this additive arithmetic is multiplicative on ratios, but I get why it could’ve been overlooked or intentionally left undefined, as it’s doubly confusing.

This is a re-implementation of the existing and poorly laid out grammar of Scale Workshop 2. I regret allowing -3/2 to mean 2/3, but that ship has sailed. I will do better in SW3 if the community allows it.

@arseniiv
Copy link

I regret allowing -3/2 to mean 2/3,

But it doesn’t (by itself as a whole row). Then it’s good. But the parser as it is allows it, so transitioning to SW3 parser should catch these places.

@frostburn
Copy link
Member Author

I regret allowing -3/2 to mean 2/3,

But it doesn’t (by itself as a whole row). Then it’s good. But the parser as it is allows it, so transitioning to SW3 parser should catch these places.

That is simply due to a fortunate bug. It was always meant to parse and has been fixed in the latest deployment.

@frostburn
Copy link
Member Author

Self-review: Seems reasonable and should make it easier slot in the new unit system in the next major version.

@frostburn frostburn merged commit 800118f into main Nov 30, 2023
1 check passed
@frostburn frostburn deleted the sw2-grammar branch November 30, 2023 03:29
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

Successfully merging this pull request may close these issues.

2 participants