Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 7 additions & 16 deletions src/Yaml/Parser.elm
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,9 @@ recordOrString indent indent_ =
let
withString string =
P.oneOf
[ P.succeed (Ast.fromString string)
[ P.succeed (Ast.Record_ (Dict.singleton ":" Ast.Null_))
|. P.end
Comment on lines +302 to +303
Copy link
Owner

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?

, P.succeed (Ast.fromString string)
|. P.end
, recordProperty indent_ string
, P.succeed (addRemaining string)
Expand All @@ -312,24 +314,13 @@ recordOrString indent indent_ =
]

addRemaining string remaining =
Ast.fromString <| U.postProcessString (removeComment string ++ remaining)

removeComment string =
string
|> String.split "#"
|> List.head
|> Maybe.withDefault ""
Ast.String_ (string ++ remaining)
Comment on lines -315 to +317
Copy link
Owner

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?

in
P.oneOf
[ quotedString indent_
, P.succeed identity
|. P.chompIf (U.neither U.isColon U.isNewLine)
|. P.chompWhile (U.neither U.isColon U.isNewLine)
|> P.getChompedString
|> P.andThen withString
[ P.succeed Ast.Null_
|. P.end
, P.succeed identity
|. P.chompWhile U.isColon
|> P.getChompedString
|= U.characters (not << U.isColon)
Copy link
Owner

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

|> P.andThen withString
]

Expand Down
19 changes: 9 additions & 10 deletions tests/TestParser.elm
Original file line number Diff line number Diff line change
Expand Up @@ -557,11 +557,10 @@ aaa: bbb"""
"""
aaa: { key1: value1, key1: value2 }
"""

-- 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_)
Comment on lines -560 to +563
Copy link
Owner

Choose a reason for hiding this comment

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

Great!

]


Expand All @@ -573,12 +572,12 @@ expectRawAst subject expected =

expectValue : String -> Ast.Value -> Expect.Expectation
expectValue subject expected =
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
Comment on lines -576 to +580
Copy link
Owner

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.



expectErr : String -> Expect.Expectation
Expand Down
Loading