Skip to content

Conversation

@chtenb
Copy link
Member

@chtenb chtenb commented Feb 20, 2022

This project could do with a more structured and comprehensive set of tests. This might especially come in handy when we want to make some fundamental changes in the near future.

@jamesdbrock I encountered some things I've flagged with TODO in the diff. Should we

  • not expose chainr1' and chainl1'?
  • change sepEndBy and endBy such that the ";" case in the following test cases succeed?
  , { name: "sepEndBy"
    , parser: mkAnyParser $ sepEndBy anyLetter (char ';')
    -- TODO: ";" should be parsed successfully
    , inputs: { successes: [ "", "a", "a;b", "a;b;c", "a;" ], failures: [ ";", ";a", "ab", "a;ab" ] }
    }
  , { name: "endBy"
    , parser: mkAnyParser $ endBy anyLetter (char ';')
    -- TODO: ";" should be parsed successfully
    , inputs: { successes: [ "", "a;", "a;b;", "a;b;c;" ], failures: [ "a", ";", ";a", "ab", "a;b", "a;b;c" ] }
    }

Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@chtenb chtenb requested a review from jamesdbrock February 20, 2022 16:22
@jamesdbrock
Copy link
Member

I agree that chainr1' and chainl1' should not be exported. That was recently fixed in parsing too. purescript-contrib/purescript-parsing#131

@jamesdbrock
Copy link
Member

I’m not sure about the ";" case for endBy and sepEndBy... I think it’s correct that it should fail to parse?

@chtenb
Copy link
Member Author

chtenb commented Feb 21, 2022

I’m not sure about the ";" case for endBy and sepEndBy... I think it’s correct that it should fail to parse?

The docs respectively say

Parse zero or more separated values, ending with a separator.

and

Parse zero or more separated values, optionally ending with a separator.

Since zero values is allowed, and the ending is also allowed (mandatory even in one of them), my reasoning is that ; should succeed.

@jamesdbrock
Copy link
Member

I just tested Parsec and the Parsec implementation agrees with your interpretation.

parse (sepEndBy letter (char ';')) "srcname" ";"
Right ""

@chtenb
Copy link
Member Author

chtenb commented Feb 21, 2022

And what about endBy anyLetter (char ';') on the input string ""? Since the sep is mandatory for endBy I would expect this to fail instead of succeed. Does that align with Parsec too?

@chtenb chtenb merged commit 50db4b0 into main Feb 26, 2022
@chtenb chtenb deleted the more_tests branch February 26, 2022 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants