-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(collections): add optional key and value naming methods #20538
Conversation
WalkthroughWalkthroughThe recent changes to the Changes
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
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.
Actionable comments posted: 17
Outside diff range and nitpick comments (9)
collections/codec/codec_test.go (1)
Line range hint
14-14
: Undefined variableErrEncoding
.Please verify that
ErrEncoding
is defined and imported. This error appears multiple times in different test cases.Also applies to: 24-24, 34-34
Tools
golangci-lint
10-10: undefined: NewUntypedValueCodec (typecheck)
collections/codec/bool.go (1)
Line range hint
26-26
: Undefined variableErrEncoding
.Please verify that
ErrEncoding
is defined and imported. This error appears in multiple methods within theboolKey
struct.Also applies to: 34-34
Tools
golangci-lint
9-9: undefined: NameableKeyCodec (typecheck)
collections/codec/string.go (1)
Line range hint
46-46
: Undefined variableErrEncoding
.Please verify that
ErrEncoding
is defined and imported. This error appears in multiple methods within thestringKey
struct.Also applies to: 57-57
Tools
golangci-lint
9-9: undefined: NameableKeyCodec (typecheck)
collections/codec/bytes.go (1)
Line range hint
54-54
: Address the undefined errorErrEncoding
.The error
ErrEncoding
is used but not defined in this file. Ensure it is imported or defined to prevent runtime errors.Also applies to: 66-66, 73-73
Tools
golangci-lint
13-13: undefined: NameableKeyCodec (typecheck)
collections/codec/int.go (1)
Line range hint
24-24
: Address the undefined errorErrEncoding
.The error
ErrEncoding
is used but not defined in this file. Ensure it is imported or defined to prevent runtime errors.Also applies to: 94-94
Tools
golangci-lint
10-10: undefined: NameableKeyCodec (typecheck)
collections/codec/uint.go (1)
Line range hint
23-23
: Address the undefined errorErrEncoding
.The error
ErrEncoding
is used but not defined in this file. Ensure it is imported or defined to prevent runtime errors.Also applies to: 82-82, 133-133
Tools
golangci-lint
10-10: undefined: NameableKeyCodec (typecheck)
collections/collections.go (2)
Line range hint
92-92
: Resolve the undefined typegenesisHandler
.The type
genesisHandler
is not defined in the provided context. Ensure that it is correctly defined and imported to avoid compilation errors.
Line range hint
138-138
: Resolve the undefined typeMap
.The type
Map
is not defined in the provided context. Ensure that it is correctly defined and imported to avoid compilation errors.collections/pair.go (1)
Line range hint
239-239
: Several undefined references toRangeKey
,RangeKeyExact
,RangeKeyNext
, andOrderDescending
are detected. These might be due to missing imports or definitions in the codebase.+ import "path/to/definitions" // Adjust the import path as necessary
Also applies to: 246-247, 256-257, 263-263, 268-268, 273-273, 278-278, 283-283
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (14)
- collections/CHANGELOG.md (1 hunks)
- collections/codec/alternative_value_test.go (1 hunks)
- collections/codec/bool.go (2 hunks)
- collections/codec/bytes.go (2 hunks)
- collections/codec/codec.go (2 hunks)
- collections/codec/codec_test.go (1 hunks)
- collections/codec/int.go (3 hunks)
- collections/codec/naming.go (1 hunks)
- collections/codec/string.go (2 hunks)
- collections/codec/uint.go (4 hunks)
- collections/collections.go (1 hunks)
- collections/naming_test.go (1 hunks)
- collections/pair.go (2 hunks)
- collections/triple.go (2 hunks)
Additional context used
Path-based instructions (14)
collections/codec/codec_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"collections/codec/naming.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/codec/alternative_value_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"collections/naming_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"collections/codec/bool.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/codec/string.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/codec/bytes.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/CHANGELOG.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"collections/codec/int.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/codec/uint.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/codec/codec.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/collections.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/pair.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/triple.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
collections/codec/codec_test.go
10-10: undefined: NewUntypedValueCodec (typecheck)
14-14: undefined: ErrEncoding (typecheck)
24-24: undefined: ErrEncoding (typecheck)
34-34: undefined: ErrEncoding (typecheck)
collections/codec/naming.go
5-5: undefined: KeyCodec (typecheck)
13-13: undefined: KeyCodec (typecheck)
22-22: undefined: ValueCodec (typecheck)
30-30: undefined: ValueCodec (typecheck)
collections/naming_test.go
10-10: undefined: Uint16Key (typecheck)
11-11: undefined: Uint32Key (typecheck)
12-12: undefined: Uint64Key (typecheck)
13-13: undefined: Int32Key (typecheck)
14-14: undefined: Int64Key (typecheck)
15-15: undefined: StringKey (typecheck)
16-16: undefined: BytesKey (typecheck)
17-17: undefined: BoolKey (typecheck)
19-19: undefined: Uint16Value (typecheck)
20-20: undefined: Uint32Value (typecheck)
21-21: undefined: Uint64Value (typecheck)
22-22: undefined: Int32Value (typecheck)
23-23: undefined: Int64Value (typecheck)
24-24: undefined: StringValue (typecheck)
25-25: undefined: BytesValue (typecheck)
26-26: undefined: BoolValue (typecheck)
28-28: undefined: NamedPairKeyCodec (typecheck)
29-29: undefined: NamedTripleKeyCodec (typecheck)
collections/codec/bool.go
9-9: undefined: NameableKeyCodec (typecheck)
70-70: undefined: NamedKeyCodec (typecheck)
26-26: undefined: ErrEncoding (typecheck)
34-34: undefined: ErrEncoding (typecheck)
collections/codec/string.go
9-9: undefined: NameableKeyCodec (typecheck)
72-72: undefined: NamedKeyCodec (typecheck)
46-46: undefined: ErrEncoding (typecheck)
57-57: undefined: ErrEncoding (typecheck)
collections/codec/bytes.go
13-13: undefined: NameableKeyCodec (typecheck)
83-83: undefined: NamedKeyCodec (typecheck)
54-54: undefined: ErrEncoding (typecheck)
66-66: undefined: ErrEncoding (typecheck)
73-73: undefined: ErrEncoding (typecheck)
collections/codec/int.go
10-10: undefined: NameableKeyCodec (typecheck)
69-69: undefined: NamedKeyCodec (typecheck)
78-78: undefined: NameableKeyCodec (typecheck)
138-138: undefined: NamedKeyCodec (typecheck)
24-24: undefined: ErrEncoding (typecheck)
94-94: undefined: ErrEncoding (typecheck)
collections/codec/uint.go
10-10: undefined: NameableKeyCodec (typecheck)
60-60: undefined: NamedKeyCodec (typecheck)
69-69: undefined: NameableKeyCodec (typecheck)
111-111: undefined: NamedKeyCodec (typecheck)
120-120: undefined: NameableKeyCodec (typecheck)
162-162: undefined: NamedKeyCodec (typecheck)
23-23: undefined: ErrEncoding (typecheck)
82-82: undefined: ErrEncoding (typecheck)
133-133: undefined: ErrEncoding (typecheck)
collections/codec/codec.go
128-128: undefined: NameableValueCodec (typecheck)
174-174: undefined: NamedValueCodec (typecheck)
collections/collections.go
92-92: undefined: genesisHandler (typecheck)
138-138: undefined: Map (typecheck)
collections/pair.go
255-255: undefined: RangeKey (typecheck)
256-256: undefined: RangeKey (typecheck)
257-257: undefined: Order (typecheck)
287-287: undefined: RangeKey (typecheck)
239-239: undefined: RangeKeyPrefixEnd (typecheck)
246-246: undefined: RangeKeyExact (typecheck)
247-247: undefined: RangeKeyPrefixEnd (typecheck)
263-263: undefined: RangeKeyExact (typecheck)
268-268: undefined: RangeKeyNext (typecheck)
273-273: undefined: RangeKeyNext (typecheck)
278-278: undefined: RangeKeyExact (typecheck)
283-283: undefined: OrderDescending (typecheck)
collections/triple.go
294-294: undefined: Ranger (typecheck)
303-303: undefined: Ranger (typecheck)
313-313: undefined: Ranger (typecheck)
296-296: undefined: Range (typecheck)
297-297: undefined: RangeKeyPrefixEnd (typecheck)
305-305: undefined: Range (typecheck)
306-306: undefined: RangeKeyExact (typecheck)
307-307: undefined: RangeKeyPrefixEnd (typecheck)
315-315: undefined: Range (typecheck)
316-316: undefined: RangeKeyExact (typecheck)
317-317: undefined: RangeKeyPrefixEnd (typecheck)
Additional comments not posted (8)
collections/CHANGELOG.md (1)
40-40
: Changelog entry is clear and informative.The changelog entry correctly summarizes the changes made in the PR, providing a clear link to the PR for more details.
collections/collections.go (1)
60-74
: Ensure proper type casting in value codec conversions.The type casting here is necessary to match the new codec interfaces. The previous comments indicate that this was done to resolve IDE errors, which is a valid reason.
collections/pair.go (3)
57-67
: The implementation ofNamedPairKeyCodec
correctly extends functionality to include naming. Ensure that the names provided are utilized appropriately in other parts of the system.
70-72
: The addition ofkey1Name
andkey2Name
topairKeyCodec
struct is consistent with the new naming functionality. This should enhance the readability and usability of the codec system.
232-234
: TheName
method implementation is straightforward and follows the expected format. Good use offmt.Sprintf
for formatting the output.collections/triple.go (3)
67-76
: The implementation ofNamedTripleKeyCodec
correctly extends functionality to include naming. Ensure that the names provided are utilized appropriately in other parts of the system.
79-82
: The addition ofkey1Name
,key2Name
, andkey3Name
totripleKeyCodec
struct is consistent with the new naming functionality. This should enhance the readability and usability of the codec system.
288-290
: TheName
method implementation is straightforward and follows the expected format. Good use offmt.Sprintf
for formatting the output.
func (t tripleKeyCodec[K1, K2, K3]) Name() string { | ||
return fmt.Sprintf("%s,%s,%s", t.key1Name, t.key2Name, t.key3Name) | ||
} | ||
|
||
// NewPrefixUntilTripleRange defines a collection query which ranges until the provided Pair prefix. | ||
// Unstable: this API might change in the future. | ||
func NewPrefixUntilTripleRange[K1, K2, K3 any](k1 K1) Ranger[Triple[K1, K2, K3]] { |
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.
Several undefined references to Ranger
, Range
, RangeKeyExact
, and RangeKeyPrefixEnd
are detected. These might be due to missing imports or definitions in the codebase.
+ import "path/to/definitions" // Adjust the import path as necessary
Also applies to: 303-303, 313-313, 296-296, 297-297, 305-305, 306-306, 307-307, 315-315, 316-316, 317-317
Committable suggestion was skipped due to low confidence.
Tools
golangci-lint
294-294: undefined: Ranger (typecheck)
@@ -7,7 +7,7 @@ import ( | |||
) | |||
|
|||
func TestUntypedValueCodec(t *testing.T) { | |||
vc := NewUntypedValueCodec(KeyToValueCodec(NewStringKeyCodec[string]())) | |||
vc := NewUntypedValueCodec(ValueCodec[string](KeyToValueCodec(KeyCodec[string](NewStringKeyCodec[string]())))) |
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.
Undefined function NewUntypedValueCodec
.
Please ensure that NewUntypedValueCodec
is defined and correctly imported.
Tools
golangci-lint
10-10: undefined: NewUntypedValueCodec (typecheck)
|
||
// NameableValueCodec is a ValueCodec that can be named. | ||
type NameableValueCodec[T any] interface { | ||
ValueCodec[T] |
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.
Undefined interface ValueCodec
.
Please ensure that ValueCodec
is defined and correctly imported. This issue affects both NameableValueCodec
and NamedValueCodec
.
Also applies to: 30-30
Tools
golangci-lint
22-22: undefined: ValueCodec (typecheck)
|
||
// NameableKeyCodec is a KeyCodec that can be named. | ||
type NameableKeyCodec[T any] interface { | ||
KeyCodec[T] |
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.
Undefined interface KeyCodec
.
Please ensure that KeyCodec
is defined and correctly imported. This issue affects both NameableKeyCodec
and NamedKeyCodec
.
Also applies to: 13-13
Tools
golangci-lint
5-5: undefined: KeyCodec (typecheck)
collections/naming_test.go
Outdated
) | ||
|
||
func TestNaming(t *testing.T) { | ||
require.Equal(t, "u16", Uint16Key.WithName("u16").Name()) |
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.
Undefined variables used in tests.
Please ensure that all keys and values (Uint16Key
, Uint32Key
, etc.) and functions (NamedPairKeyCodec
, NamedTripleKeyCodec
) are defined and correctly imported. This issue affects multiple lines in the test cases.
Also applies to: 11-11, 12-12, 13-13, 14-14, 15-15, 16-16, 17-17, 19-19, 20-20, 21-21, 22-22, 23-23, 24-24, 25-25, 26-26, 28-28, 29-29
Tools
golangci-lint
10-10: undefined: Uint16Key (typecheck)
@@ -7,9 +7,11 @@ import ( | |||
"strconv" | |||
) | |||
|
|||
func NewUint64Key[T ~uint64]() KeyCodec[T] { return uint64Key[T]{} } | |||
func NewUint64Key[T ~uint64]() NameableKeyCodec[T] { return uint64Key[T]{} } |
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.
Resolve the undefined type NameableKeyCodec
.
The type NameableKeyCodec
is not defined in the provided context. Ensure that it is correctly defined and imported to avoid compilation errors.
Also applies to: 69-69, 120-120
Tools
golangci-lint
10-10: undefined: NameableKeyCodec (typecheck)
collections/codec/uint.go
Outdated
@@ -55,9 +57,20 @@ func (uint64Key[T]) KeyType() string { | |||
return "uint64" | |||
} | |||
|
|||
func NewUint32Key[T ~uint32]() KeyCodec[T] { return uint32Key[T]{} } | |||
func (u uint64Key[T]) WithName(name string) NamedKeyCodec[T] { |
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.
Resolve the undefined type NamedKeyCodec
.
The type NamedKeyCodec
is not defined. Ensure that it is correctly defined and imported to avoid compilation errors.
Also applies to: 111-111, 162-162
Tools
golangci-lint
60-60: undefined: NamedKeyCodec (typecheck)
@@ -125,11 +125,14 @@ type UntypedValueCodec struct { | |||
} | |||
|
|||
// KeyToValueCodec converts a KeyCodec into a ValueCodec. | |||
func KeyToValueCodec[K any](keyCodec KeyCodec[K]) ValueCodec[K] { return keyToValueCodec[K]{keyCodec} } | |||
func KeyToValueCodec[K any](keyCodec KeyCodec[K]) NameableValueCodec[K] { |
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.
Resolve the undefined types NameableValueCodec
and NamedValueCodec
.
The types NameableValueCodec
and NamedValueCodec
are not defined in the provided context. Ensure that they are correctly defined and imported to avoid compilation errors.
Also applies to: 174-174
Tools
golangci-lint
128-128: undefined: NameableValueCodec (typecheck)
collections/pair.go
Outdated
// NamedPairKeyCodec instantiates a new KeyCodec instance that can encode the Pair, given the KeyCodec of the | ||
// first part of the key and the KeyCodec of the second part of the key, with names assigned to each part | ||
// which will only be used for indexing and informational purposes. | ||
func NamedPairKeyCodec[K1, K2 any](key1Name string, keyCodec1 codec.KeyCodec[K1], key2Name string, keyCodec2 codec.KeyCodec[K2]) codec.NamedKeyCodec[Pair[K1, K2]] { |
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 was thinking that PairKeycodec can attempt to cast the codec to a named one?
Would love to see how this might look in a module even in a throwaway branch
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.
See bank in #20547
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
// NamedPairKeyCodec instantiates a new KeyCodec instance that can encode the Pair, given the KeyCodec of the | ||
// first part of the key and the KeyCodec of the second part of the key. | ||
// It also provides names for the keys which are used for indexing purposes. | ||
func NamedPairKeyCodec[K1, K2 any](key1Name string, keyCodec1 codec.KeyCodec[K1], key2Name string, keyCodec2 codec.KeyCodec[K2]) codec.KeyCodec[Pair[K1, K2]] { |
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.
not blocking but maybe we should just pass here a namedKeyCodec
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.
Not a bad idea. What would you prefer?
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.
Overall lgtm, wondering about the brreaking changes on globals, they should not be felt.. but still
collections/CHANGELOG.md
Outdated
@@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
* [#19861](https://github.com/cosmos/cosmos-sdk/pull/19861) Add `NewJSONValueCodec` value codec as an alternative for `codec.CollValue` from the SDK for non protobuf types. | |||
* [#21090](https://github.com/cosmos/cosmos-sdk/pull/21090) Introduces `Quad`, a composite key with four keys. | |||
* [#20704](https://github.com/cosmos/cosmos-sdk/pull/20704) Add `ModuleCodec` method to `Schema` and `HasSchemaCodec` interface in order to support `cosmossdk.io/schema` compatible indexing. | |||
* [#20538](https://github.com/cosmos/cosmos-sdk/pull/20538) Add `Nameable` and `Named` variations to `KeyCodec` and `ValueCodec` to allow for better indexing of collections types. |
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.
This should be marked as API breaking
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.
How is it API breaking?
Description
This adds the ability to name key and value codecs aligned with the way schema works. Most naming of simple codec's happens through
NamedKeyCodec
andNamedValueCodec
wrappers. There are special implementations for pair, triple and quads - encoding details for those types will be handled in a separate PR.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.md
Reviewers 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
Summary by CodeRabbit
New Features
Nameable
andNamed
variations to key and value codecs to enhance indexing capabilities.Bug Fixes
Refactor