feat(collections): add NewJSONValueCodec#19861
Conversation
WalkthroughThe update aims to enhance the collections library by introducing new data structures like Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (2)
- collections/CHANGELOG.md (2 hunks)
- collections/json.go (1 hunks)
Additional Context Used
Path-based Instructions (2)
collections/json.go (1)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.collections/CHANGELOG.md (1)
**/*.md:
Assess the documentation for misspellings, grammatical errors, missing documentation and correctness
Additional comments not posted (9)
collections/json.go (6)
10-14: TheNewJSONValueCodecfunction correctly implements the factory pattern for creating a new JSON value codec. It uses reflection to obtain the type name ofT, which is stored for later use. This approach is efficient and follows best practices for generic type handling in Go.
16-18: ThejsonValuestruct is minimalistic and serves its purpose well by storing the type name of the generic typeT. This design choice keeps the struct lightweight and focused on its role in the codec implementation.
20-28: TheDecodemethod correctly implements thecodec.ValueCodecinterface for decoding JSON bytes into a Go value of typeT. It uses the standardjson.Unmarshalfunction, which is the correct choice for JSON decoding in Go. Error handling is appropriately implemented, returning the error to the caller if unmarshalling fails.
40-43: TheEncodemethod correctly implements thecodec.ValueCodecinterface for encoding a Go value of typeTinto JSON bytes. It uses the standardjson.Marshalfunction, which is the appropriate choice for JSON encoding in Go. The method is concise and correctly handles the encoding process.
50-53: TheStringifymethod provides a simple way to convert a value of typeTinto a string representation. It usesfmt.Sprintfwith the%vverb, which is a reasonable choice for a generic string representation. This method enhances the usability of the codec by allowing easy logging or debugging of values.
55-58: TheValueTypemethod correctly implements thecodec.ValueCodecinterface, providing a string representation of the value type handled by the codec. It combines the "json" prefix with the stored type name, offering clear and informative output about the codec's target type.collections/CHANGELOG.md (3)
45-49: The introduction ofTripleas a composite key and the clarification on collections iteration methods are well-documented. These entries are clear, link to the relevant pull requests, and correctly categorized under "Features" and "API Breaking" respectively.
59-62: The addition of theClearmethod forMapandKeySet, along with the introduction ofAltValueCodec, are significant enhancements. The entries are clear but could benefit from a brief description ofAltValueCodec's purpose and use cases for clarity.Consider adding a brief description of
AltValueCodecto clarify its purpose and use cases.
68-72: The decision to make the genericCollectioninterface public is a notable change, correctly documented under "Features" and "API Breaking". The entries are clear and provide direct links to the relevant pull requests, facilitating easy access to more detailed information.
| // DecodeJSON implements codec.ValueCodec. | ||
| func (jsonValue[T]) DecodeJSON(b []byte) (T, error) { | ||
| var t T | ||
| if err := json.Unmarshal(b, &t); err != nil { | ||
| return t, err | ||
| } | ||
|
|
||
| return t, nil | ||
| } |
There was a problem hiding this comment.
The DecodeJSON method appears to be a duplicate of the Decode method, performing the same operation. This redundancy could lead to confusion and maintenance issues. Consider removing DecodeJSON or clarifying its intended use case if it differs from Decode.
- // DecodeJSON implements codec.ValueCodec.
- func (jsonValue[T]) DecodeJSON(b []byte) (T, error) {
- var t T
- if err := json.Unmarshal(b, &t); err != nil {
- return t, err
- }
-
- return t, nil
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // DecodeJSON implements codec.ValueCodec. | |
| func (jsonValue[T]) DecodeJSON(b []byte) (T, error) { | |
| var t T | |
| if err := json.Unmarshal(b, &t); err != nil { | |
| return t, err | |
| } | |
| return t, nil | |
| } |
| // EncodeJSON implements codec.ValueCodec. | ||
| func (jsonValue[T]) EncodeJSON(value T) ([]byte, error) { | ||
| return json.Marshal(value) | ||
| } |
There was a problem hiding this comment.
The EncodeJSON method, similar to DecodeJSON, appears to be a duplicate of the Encode method, performing the same operation. This redundancy could lead to confusion and maintenance issues. Consider removing EncodeJSON or clarifying its intended use case if it differs from Encode.
- // EncodeJSON implements codec.ValueCodec.
- func (jsonValue[T]) EncodeJSON(value T) ([]byte, error) {
- return json.Marshal(value)
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // EncodeJSON implements codec.ValueCodec. | |
| func (jsonValue[T]) EncodeJSON(value T) ([]byte, error) { | |
| return json.Marshal(value) | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- collections/CHANGELOG.md (2 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"
| * [#19343](https://github.com/cosmos/cosmos-sdk/pull/19343) Simplify IndexedMap creation by allowing to infer indexes through reflection. | ||
| * [#18933](https://github.com/cosmos/cosmos-sdk/pull/18933) Add LookupMap implementation. It is basic wrapping of the standard Map methods but is not iterable. | ||
| * [#17656](https://github.com/cosmos/cosmos-sdk/pull/17656) Introduces `Vec`, a collection type that allows to represent a growable array on top of a KVStore. | ||
| * [#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. |
There was a problem hiding this comment.
Ensure consistency in spacing after the PR links in the changelog entries.
- * [#19343](https://github.com/cosmos/cosmos-sdk/pull/19343) Simplify IndexedMap creation by allowing to infer indexes through reflection.
+ * [#19343](https://github.com/cosmos/cosmos-sdk/pull/19343) Simplify IndexedMap creation by allowing to infer indexes through reflection.
- * [#18933](https://github.com/cosmos/cosmos-sdk/pull/18933) Add LookupMap implementation. It is basic wrapping of the standard Map methods but is not iterable.
+ * [#18933](https://github.com/cosmos/cosmos-sdk/pull/18933) Add LookupMap implementation. It is basic wrapping of the standard Map methods but is not iterable.
- * [#17656](https://github.com/cosmos/cosmos-sdk/pull/17656) Introduces `Vec`, a collection type that allows to represent a growable array on top of a KVStore.
+ * [#17656](https://github.com/cosmos/cosmos-sdk/pull/17656) Introduces `Vec`, a collection type that allows to represent a growable array on top of a KVStore.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * [#19343](https://github.com/cosmos/cosmos-sdk/pull/19343) Simplify IndexedMap creation by allowing to infer indexes through reflection. | |
| * [#18933](https://github.com/cosmos/cosmos-sdk/pull/18933) Add LookupMap implementation. It is basic wrapping of the standard Map methods but is not iterable. | |
| * [#17656](https://github.com/cosmos/cosmos-sdk/pull/17656) Introduces `Vec`, a collection type that allows to represent a growable array on top of a KVStore. | |
| * [#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. | |
| * [#19343](https://github.com/cosmos/cosmos-sdk/pull/19343) Simplify IndexedMap creation by allowing to infer indexes through reflection. | |
| * [#18933](https://github.com/cosmos/cosmos-sdk/pull/18933) Add LookupMap implementation. It is basic wrapping of the standard Map methods but is not iterable. | |
| * [#17656](https://github.com/cosmos/cosmos-sdk/pull/17656) Introduces `Vec`, a collection type that allows to represent a growable array on top of a KVStore. | |
| * [#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. |
* main: refactor(x/simulation)!: remove accounts string (cosmos#20056) fix(baseapp): don't share global gas meter in tx execution (cosmos#19616) feat(x/accounts): use router service from env (cosmos#20003) refactor(x): remove Address.String() (cosmos#20048) build(deps): Bump github.com/jhump/protoreflect from 1.15.6 to 1.16.0 in /tests (cosmos#20040) feat(collections): add `NewJSONValueCodec` (cosmos#19861)
4 participants
Description
ref: #19788 (comment)
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.
I have...
Summary by CodeRabbit
IndexedMap.LookupMapfor enhanced data lookup.Vecfor dynamic array support.JSONValueCodecfor improved JSON encoding/decoding.Triplefor composite key functionality.Clearmethod for easier data removal inMapandKeySet.Collectioninterface public for broader usability.