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

Replace UMapCompact with UMap and add tests #3371

Merged
merged 6 commits into from
Apr 19, 2023
Merged

Replace UMapCompact with UMap and add tests #3371

merged 6 commits into from
Apr 19, 2023

Conversation

aniketd
Copy link
Contributor

@aniketd aniketd commented Apr 5, 2023 •
edited
Loading

Description

Closes #3272

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the CHANGELOG.md for affected package
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@aniketd aniketd requested review from JaredCorduan and lehins April 5, 2023 15:00
@aniketd
Copy link
Contributor Author

aniketd commented Apr 5, 2023

2 tests are failing still, unexpectedly. I'm looking into it 👍

JaredCorduan
JaredCorduan approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

love it! thank you!

@aniketd
Copy link
Contributor Author

aniketd commented Apr 6, 2023

Those tests should now pass 🤞

@aniketd aniketd force-pushed the aniketd/remove-data-umap branch 2 times, most recently from 102433e to b4abf6f Compare April 11, 2023 12:25
lehins
lehins requested changes Apr 13, 2023
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.

Looking great. Some issues with versions and bounds, but this is a totally new practice for everyone on the team, so it will take a while to get used to.

libs/cardano-ledger-core/src/Cardano/Ledger/UMap.hs Show resolved Hide resolved
libs/cardano-ledger-core/test/Test/Cardano/Ledger/UMapSpec.hs Show resolved Hide resolved
libs/cardano-ledger-core/test/Test/Cardano/Ledger/UMapSpec.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/test/Test/Cardano/Ledger/UMapSpec.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/test/Test/Cardano/Ledger/UMapSpec.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Arbitrary.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Arbitrary.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/UMap.hs Show resolved Hide resolved
libs/cardano-data/cardano-data.cabal Show resolved Hide resolved
libs/set-algebra/src/Control/Iterate/BaseTypes.hs Show resolved Hide resolved
@Soupstraw Soupstraw requested a review from lehins April 17, 2023 10:55
@aniketd aniketd force-pushed the aniketd/remove-data-umap branch 2 times, most recently from 3af4945 to d5f4225 Compare April 17, 2023 11:41
@aniketd
Copy link
Contributor Author

aniketd commented Apr 17, 2023

@lehins This is ready for another review!

lehins
lehins approved these changes Apr 18, 2023
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.

Awesome work. Thank you.

libs/cardano-ledger-core/CHANGELOG.md Outdated Show resolved Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/UMap.hs Show resolved Hide resolved
@aniketd aniketd force-pushed the aniketd/remove-data-umap branch from d5f4225 to e144f2c Compare April 18, 2023 11:01
@lehins lehins force-pushed the aniketd/remove-data-umap branch from e144f2c to 405c245 Compare April 18, 2023 11:48
@lehins lehins enabled auto-merge April 18, 2023 11:48
@lehins lehins disabled auto-merge April 18, 2023 12:56
@lehins
Copy link
Collaborator

lehins commented Apr 18, 2023

@aniketd Looks like there is a test failure

aniketd added 6 commits April 19, 2023 13:03
@aniketd
Remove 'Data.UMap'.
12d7d3c 
- Also, remove use of it from set-algebra. Most of it were unused instances.
- Bump versions of both `cardano-data` and `set-algebra` and update their changelogs.

This is in preparation for renaming `Cardano.Ledger.UMapCompact` back to `UMap` in
subsequent commits.
@aniketd
Rename Cardano.Ledger.UMapComact to Cardano.Ledger.UMap.
cf0d17a 
Also, bump `cardano-ledger-core` version to 1.2.0.0
@aniketd
Rename UMapCompactSpec to UMapSpec.
7a4c6bf 
These tests are not likely to compile without errors yet, so just
the file is moved, but not added to the cabal file.
@aniketd
Update UMapSpec tests and add them to cabal.
c40af67 
Add missing required arbitrary instances.
@aniketd
Add UMapComact as deprecated. Update changelog for entire PR.
1766531 
The versions don't need bumping as we have already done that in a
previous commit.
@aniketd
Fix bug in UMap.insertWith' and generators for insert-delete roundtrip
72d1953
@aniketd aniketd force-pushed the aniketd/remove-data-umap branch from 405c245 to 72d1953 Compare April 19, 2023 07:33
@aniketd aniketd requested a review from lehins April 19, 2023 07:35
lehins
lehins approved these changes Apr 19, 2023
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.

👍

@lehins lehins merged commit 18a747a into master Apr 19, 2023
@iohk-bors iohk-bors bot deleted the aniketd/remove-data-umap branch April 19, 2023 11:32
@aniketd aniketd mentioned this pull request Apr 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaredCorduan JaredCorduan JaredCorduan approved these changes

@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.

Remove Data.UMap
3 participants
@aniketd @lehins @JaredCorduan