Skip to content

Fix #914. #950

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 1 commit into from
Jan 20, 2017
Merged

Fix #914. #950

merged 1 commit into from
Jan 20, 2017

Conversation

bsansouci
Copy link
Contributor

This is a pretty simple fix.
I use the existent parameter withStringKeys to when to format the key
as an ident, or as a simple string.

For now we don't support Ldot and Lapply in the string keys. I'm not sure what the behavior should be in this case.

cc @chenglou

This is a pretty simple fix.
I use the existent parameter `withStringKeys` to when to format the key
as an ident, or as a simple string.
@chenglou
Copy link
Member

cc @yunxing

@chenglou
Copy link
Member

every weird string key should be supported though; Is that hard? I believe the same will apply for #941

@bsansouci
Copy link
Contributor Author

@chenglou I think Ldot and Lapply are kinds of identifier. In this case, we parse the key as an Lident https://github.com/facebook/reason/blob/c2661cabc1405b373a84047a61afffd0e121aef2/src/reason_parser.mly#L3483. So there's no way (right?) to have anything else in those case. That'll support any string (because we parse any string).

@yunxing
Copy link
Contributor

yunxing commented Jan 17, 2017

Accidentally lost track on this, looking into it now.

@yunxing
Copy link
Contributor

yunxing commented Jan 20, 2017

Very sorry for the delay. Diff looks good. Let's merge this!

@yunxing yunxing merged commit 0fd5375 into reasonml:master Jan 20, 2017
sansthesis pushed a commit to sansthesis/reason that referenced this pull request Apr 19, 2017
This is a pretty simple fix.
I use the existent parameter `withStringKeys` to when to format the key
as an ident, or as a simple string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants