Skip to content

Add basic spec tests #84

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 6 commits into from
Feb 26, 2022
Merged

Add basic spec tests #84

merged 6 commits into from
Feb 26, 2022

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.

2 participants