-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix for issue #11 #39
base: master
Are you sure you want to change the base?
Fix for issue #11 #39
Conversation
I added a test for you, which is now failing. |
@@ -551,10 +551,8 @@ recordInlinePropertyName = | |||
|
|||
recordInlinePropertyNameString : P.Parser String | |||
recordInlinePropertyNameString = | |||
-- TODO allow numeric name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer you not remove TODO comments that your PR does not address.
P.succeed () | ||
|. P.chompWhile (U.neither3 U.isColon U.isComma U.isRecordEnd) | ||
|. U.whitespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is incorrect. Whitespace here should be ignored
@@ -581,7 +579,7 @@ recordInlineString = | |||
P.succeed () | |||
|. P.chompWhile (U.neither U.isComma U.isRecordEnd) | |||
|> P.getChompedString | |||
|> P.map Ast.fromString | |||
|> P.map (Ast.fromString << String.trim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why you're doing this? I don't see how this addresses the issue?
Description of the change
Clearly and concisely describe the purpose of the pull request. If this PR relates to an existing issue or change proposal, please link to it. Include any other background context that would help reviewers understand the motivation for this PR.
Checklist: