Closed larskuhtz closed 10 years ago
I think what we may want to do is move the 'prop>'s out into a separate quick check property test suite.
That would avoid breaking users who actually need the --hide-all-packages
protection.
With that change I'm 100% behind this fix.
I think a quick check property test suite would be a good solution. I was using doctest just because it was already there and I didn't want to introduce a new style of testing.
Do you prefer the properties to go into a dedicated test module (under ./test) or in the module with the tested code?
I moved the test code of this patch into a new quick check test suite at ./tests/QuickCheck and updated the pull request.
I tend to prefer QC to go out in the test folder alongside the doctest suite, like you've done here.
If you look at lens it has a few test suites in that style.
Looks good to me.
The Alternative instances for Parsec, Attoparsec, and ReadP behave all different:
Parsec: non-backtracking and non-commutative Attoparsec: backtracking and non-commutative ReadP: backtracking and commutative
The current default implementation is correct only for a non-backtracking and commutative behavior of
<|>
. Ironically, the Parsing instance for Parsec provides its own implementation of notFollowedBy whereas Attoparsec and ReadP use the default implementation which does not behave correctly for these.This patch provides implementations of
notFollowedBy
for Attoparsec and ReadP. Due to the lack of an explicit non-commutative choice by means of primitives from the Parsing class there is no default implementation that would work for all parsing frameworks. Therefor the default implementation is removed by this patch. Due to this removalShow
constraints are required for the state parameters of theParsing
instances forStateT
,WriteT
, andRWST
.