-
Notifications
You must be signed in to change notification settings - Fork 157
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
Added property tests for the KeyMap data structure. #2552
Conversation
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've looked through a portion of this PR, made few minor suggestion, but haven't finished it yet. Over all it looks great. I am requesting some changes before I review it again just because this PR includes much code that is not ready for master yet, namely bulk insertion. I left a comment about it in the review.
Problem with ormolu is that we now use newer version on CI, make sure you have ormolu-0.3.1.0
installed. I think doing cabal install ormolu
should do the trick
fceb3cf
to
60bb5a4
Compare
60bb5a4
to
c80224b
Compare
Rebased on master, and all conversations resolved |
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.
It should be possible to rewrite all those cases that use head
with something like listToMaybe
or direct pattern matching. Worst case scenario we can use error
, as it provides location of the error and information about why it should have been impossible.
c80224b
to
ae14210
Compare
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.
Beautiful!
ae14210
to
b0901f1
Compare
b0901f1
to
170e25b
Compare
Adds property tests for KeyMap. Also adds some tools for computing statistics about a KeyMap.