Skip to content

Implement Syntax 0.8, part 1 (items 1-5) #307

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

Merged
merged 5 commits into from
Nov 22, 2018

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Nov 16, 2018

This implements changes listed as 1 through 5 from #303.

@stasm stasm mentioned this pull request Nov 16, 2018
15 tasks
Copy link
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

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

I glanced over this, but didn't find the time to do an actual review.

The comments changes here change again, I think?

Also, send an inquiry to @flodolo that we're OK to unsupport the text run changes without a compat implementation?

// The Fluent Syntax spec uses /.*/ to parse comment lines. It matches all
// characters except the following ones, which are considered line endings by
// the regex engine.
const COMMENT_EOL = ["\n", "\r", "\u2028", "\u2029"];
Copy link
Contributor

Choose a reason for hiding this comment

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

I just reviewed the opposite of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is only for part 1 of #303. The change you're talking about, projectfluent/fluent#219, will be implemented later on. I chose this approach to minimize the amount of work related to porting reference tests to fluent.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, const COMMENT_EOL = ["\n", "\r", "\u2028", "\u2029"]; corresponds to the Fluent Syntax spec as of projectfluent/fluent@bf46e18. I'll remove it before all of Syntax 0.8 is implemented.

@zbraniecki zbraniecki self-requested a review November 22, 2018 01:45
Copy link
Collaborator

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

lgtm!

if (source[cursor] === "}") {
throw new FluentError("Unbalanced closing brace");
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit only because it's hot. You can store source[cursor] and if/else here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I think I prefer the current version, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(There's no impact on the perf benchmark and the current version is quicker to read. I also wouldn't be surprised if JS engines optimized this anyways to some extend.)

@stasm stasm changed the base branch from master to zeroeight November 22, 2018 11:27
@stasm stasm merged commit fd37e32 into projectfluent:zeroeight Nov 22, 2018
@stasm stasm deleted the zeroeight-part1 branch November 22, 2018 11:27
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.

3 participants