-
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 bug with : #40
base: master
Are you sure you want to change the base?
Fix bug with : #40
Conversation
[ P.succeed (Ast.Record_ (Dict.singleton ":" Ast.Null_)) | ||
|. P.end |
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 doesn't seem right. Where is the parser consuming a ":" character?
Ast.fromString <| U.postProcessString (removeComment string ++ remaining) | ||
|
||
removeComment string = | ||
string | ||
|> String.split "#" | ||
|> List.head | ||
|> Maybe.withDefault "" | ||
Ast.String_ (string ++ remaining) |
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 seems unrelated? Why is removeComment
being removed?
, P.succeed identity | ||
|. P.chompWhile U.isColon | ||
|> P.getChompedString | ||
|= U.characters (not << U.isColon) |
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 think I see what you're going for here but Yaml.Parser.Util
doesn't export the characters
function. Consider exporting that function for use here
|
||
-- TODO: This is temporarily removed because it is a valid test case that should pass | ||
-- , Test.test "weird colon record" <| | ||
-- \_ -> | ||
-- expectValue "::" <| Ast.Record_ (Dict.singleton ":" Ast.Null_) | ||
, Test.test "colon as key" <| | ||
\_ -> | ||
expectValue "::" <| | ||
Ast.Record_ (Dict.singleton ":" Ast.Null_) |
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.
Great!
case ( Parser.fromString subject, expected ) of | ||
( Ok (Ast.Float_ got), Ast.Float_ want ) -> | ||
expectCloseTo got want | ||
case (Parser.fromString subject, expected) of | ||
(Ok got, _) -> | ||
Expect.equal got expected | ||
|
||
( got, _ ) -> | ||
Expect.equal got (Ok expected) | ||
(Err err, _) -> | ||
Expect.fail err |
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 is incorrect. Comparing floating point values for equality should be done more carefully. Also, such a change should be done in another PR since it is unrelated to colon keys. Once again, I think I see where you're going and, in principle, I would accept this function being cleaned up a bit but please do this in a separate RP.
This pull request addresses the issue where the YAML parser does not correctly handle cases where a colon (:) is used as a key. The change ensures that the parser produces an Ast.Record_ with the key : and value Ast.Null_ when encountering such cases. This fix is related to the issue #10 (link to the issue).