Skip to content

Restrict variant keys #157 #268

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 3 commits into from
Aug 30, 2018
Merged

Conversation

unclenachoduh
Copy link
Contributor

Restrict variant keys to Identifiers and NumberLiterals #157

Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

Thanks, @unclenachoduh. There are some extra files included in this PR which look like they were accidentally added with git add. Otherwise, the change looks good technically.

I can't merge this just yet, however. The decision to make this change to the Fluent Syntax Spec has not yet been made. I'm still gathering feedback on the proposal in projectfluent/fluent#127. Please comment there if you think this proposal should be accepted.

@unclenachoduh
Copy link
Contributor Author

Ah, yes. I just removed those files from the PR.

I was aware that this PR might not be able to merge. I took it as an opportunity to get to know fluent.js. Hopefully this PR becomes useful if the decision regarding that issue goes in this direction. Thanks, Stas.

exit Outdated
+ // });

test("variant key number", function() {
const input = ftl`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file was added by accident, please remove.

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.

that looks good! Thank you :)

@stasm stasm changed the base branch from master to zeroseven August 10, 2018 08:42
Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

Let's merge this into zeroseven. We'll need to add a few more tests from the reference parser, but it's going to be easier to do once other changes have been implemented. Thanks, @unclenachoduh, for the PR!

@stasm stasm merged commit ca878e0 into projectfluent:zeroseven Aug 30, 2018
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