feat(collections): add collections reverse triple helpers #22641
Conversation
oren-lava
requested review from
testinginprod,
facundomedica and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce three new methods in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Collections
participant Ranger
User->>Collections: Call NewPrefixedTripleRangeReversed()
Collections->>Ranger: Create Ranger with descending order
Ranger-->>Collections: Return Ranger
Collections-->>User: Return Ranger for reversed range
sequenceDiagram
participant User
participant Collections
participant Ranger
User->>Collections: Call NewSuperPrefixedTripleRangeReversed()
Collections->>Ranger: Create Ranger with descending order
Ranger-->>Collections: Return Ranger
Collections-->>User: Return Ranger for reversed range
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
collections/triple_test.go (2)
48-56: Consider making the reverse order verification more explicit.While the current implementation correctly verifies the reverse order, consider making it more explicit using a dedicated helper or clearer variable names. For example:
-for i := range gotKeys { - require.Equal(t, gotKeys[i], keys[len(gotKeys)-i-1]) -} +expectedReversedKeys := make([]collections.Triple[uint64, string, []byte], len(keys[:3])) +for i := range keys[:3] { + expectedReversedKeys[i] = keys[2-i] +} +require.Equal(t, expectedReversedKeys, gotKeys)
65-73: Consider adding edge cases and using table-driven tests.The current tests cover the happy path well, but consider adding:
- Empty range scenarios
- Single element ranges
- Table-driven tests to make test cases more maintainable
Example structure:
testCases := []struct { name string setup []collections.Triple[uint64, string, []byte] prefix uint64 superPrefix string want []collections.Triple[uint64, string, []byte] }{ { name: "empty range", // ... test case details }, // ... more test cases }collections/triple.go (1)
378-387: LGTM! Consider adding test helpers for common test scenarios.The implementation correctly implements reverse iteration for super-prefixed ranges. To improve test maintainability, consider creating test helpers that can verify both forward and reverse iterations with the same test data.
Example test helper structure:
func assertTripleRange[K1, K2, K3 any](t *testing.T, ranger Ranger[Triple[K1, K2, K3]], expected []Triple[K1, K2, K3]) { // Common assertions for both forward and reverse ranges }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
collections/triple.go(1 hunks)collections/triple_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
collections/triple.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/triple_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
collections/triple_test.go (1)
10-10: LGTM! Clear and meaningful import alias.
The alias coretesting clearly distinguishes the core testing package from the standard testing package, improving code readability.
collections/triple.go (2)
367-376: LGTM! Consistent implementation of reverse iteration.
The implementation correctly maintains the same range boundaries while adding descending order support.
356-365: LGTM! Implementation matches the forward iteration pattern.
The implementation correctly mirrors the non-reversed version while adding the descending order flag. The unstable API warning is appropriate for this new feature.
Let's verify the test coverage:
julienrbrt
changed the title
done :) @julienrbrt |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
collections/CHANGELOG.md (2)
36-36: Enhance the changelog entry description.While the entry follows the correct format, consider expanding it to be more descriptive about the specific functionality added. For example:
-* [#22641](https://github.com/cosmos/cosmos-sdk/pull/22641) Add reverse iterator support for `Triple`. +* [#22641](https://github.com/cosmos/cosmos-sdk/pull/22641) Add reverse iterator support for `Triple` with new methods `NewPrefixUntilTripleRangeReversed`, `NewPrefixedTripleRangeReversed`, and `NewSuperPrefixedTripleRangeReversed` to support descending order in Triple collections.
35-35: Consider adding a placeholder release date.For better tracking, consider adding a placeholder date in the Unreleased section:
## [Unreleased] +<!-- Release date: TBD -->
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
collections/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
collections/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (1)
collections/CHANGELOG.md (1)
Line range hint 1-35: LGTM! Well-structured changelog format.
The changelog follows the Keep a Changelog format correctly, with clear organization of versions and proper categorization of changes.
|
Thanks for sticking around! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Closes: #XXXX
Added reverse triple helpers to support descending order for Triple
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Triplekeys in the collections package.VecandLookupMap, along with a composite key typeQuad.NewJSONValueCodec, for improved data handling.