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.

Sign up for GitHub

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

Jump to bottom

Added property tests for the KeyMap data structure. #2552

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Added property tests for the KeyMap data structure. #2552

merged 1 commit into from
Nov 29, 2021

Conversation

TimSheard
Copy link
Contributor

Adds property tests for KeyMap. Also adds some tools for computing statistics about a KeyMap.

lehins
lehins requested changes Nov 15, 2021
View reviewed changes
Copy link
Collaborator

@lehins lehins left a 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

libs/compact-map/test/Test/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/test/Test/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/test/Test/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/test/Test/Compact/KeyMap.hs Show resolved Hide resolved
libs/compact-map/test/Test/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/src/Data/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/src/Data/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/src/Data/Compact/KeyMap.hs Show resolved Hide resolved
libs/compact-map/src/Data/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/src/Data/Compact/KeyMap.hs Outdated Show resolved Hide resolved
@TimSheard TimSheard force-pushed the ts-keymap-proptests branch 3 times, most recently from fceb3cf to 60bb5a4 Compare November 17, 2021 16:39
@TimSheard
Copy link
Contributor Author

Rebased on master, and all conversations resolved

lehins
lehins requested changes Nov 23, 2021
View reviewed changes
Copy link
Collaborator

@lehins lehins left a 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.

libs/compact-map/src/Data/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/src/Data/Compact/KeyMap.hs Outdated Show resolved Hide resolved
libs/compact-map/src/Data/Compact/KeyMap.hs Outdated Show resolved Hide resolved
lehins
lehins approved these changes Nov 24, 2021
View reviewed changes
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Beautiful!

@TimSheard TimSheard merged commit 13c53f5 into master Nov 29, 2021
@iohk-bors iohk-bors bot deleted the ts-keymap-proptests branch November 29, 2021 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lehins lehins lehins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TimSheard @lehins