-
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(version): Add extraInfo to version cmd #18063
Conversation
simapp/simd/cmd/commands.go
Outdated
cmdContext := context.WithValue(rootCmd.Context(), "extraInfo", extraInfo) | ||
rootCmd.SetContext(cmdContext) | ||
|
||
rootCmd.AddCommand(version.NewVersionCommand()) |
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.
we might not need this as it's already being added by server.AddCommands
simapp/simd/cmd/commands.go
Outdated
) { | ||
cfg := sdk.GetConfig() | ||
cfg.Seal() | ||
|
||
cmdContext := context.WithValue(rootCmd.Context(), "extraInfo", extraInfo) |
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.
the context key should probably be a struct instead of a string.
simapp/simd/cmd/root_v2.go
Outdated
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.
you are missing an update in root.go, that's why the build is failing
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.
can we get a paragraph in docs for this and upgrading.md entry?
IMHO, there is no upgrading.md needed, as it is an optional feature. We could revert everything from simapp here, and let people who want to use it, do that directly in their app. Imho, given that we set empty extra info, this is the best to revert simapp changes and add good go doc above |
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.
utACK. Let's add some extended go doc in NewVersionCommand
.
gentle ping on adding this feature into the docs |
WalkthroughThis update introduces a new feature to the Changes
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- version/command.go (3 hunks)
- version/version.go (3 hunks)
- version/version_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional comments (Suppressed): 2
version/version.go (2)
26-28: The
ContextKey
struct is defined but not used anywhere in the provided hunks. Ensure that it is used in the rest of the codebase, otherwise, it's an unnecessary declaration.74-74: The
ExtraInfo
field is added to theInfo
struct. This is a good way to extend the existing structure with additional information. However, it's important to ensure that all code that creates or manipulatesInfo
objects is updated to handle theExtraInfo
field correctly. Also, sinceExtraInfo
is a pointer, make sure to check fornil
before accessing it to avoid potential runtime errors.
// Note: When seeking to add the extra info to the context | ||
// The below can be added to the initRootCmd to include the extraInfo field | ||
// | ||
// cmdContext := context.WithValue(context.Background(), version.ContextKey{}, extraInfo) | ||
// rootCmd.SetContext(cmdContext) |
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.
The comment provides instructions on how to add extra info to the context. However, it would be more maintainable and less error-prone if this was handled by the function itself, rather than requiring the caller to do it. Consider refactoring NewVersionCommand
to accept an ExtraInfo
parameter, which it can then add to the context.
// Extract and set extra information from the context | ||
extraInfo := extraInfoFromContext(cmd) | ||
verInfo.ExtraInfo = &extraInfo |
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.
The extraInfoFromContext
function is used to extract extra information from the context. However, there is no error handling if the context does not contain ExtraInfo
. This could lead to unexpected behavior if the context is not properly initialized. Consider adding error handling or a default value for ExtraInfo
.
// ExtraInfo contains a set of extra information provided by apps | ||
type ExtraInfo map[string]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.
The ExtraInfo
type is defined as a map of string to string. This is a flexible structure that can hold various types of information. However, it might be worth considering if a more structured approach would be beneficial, especially if there are specific pieces of information that will always be included in the ExtraInfo
. If that's the case, a struct with specific fields might be a better choice.
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.
please add mention of this in the docs
@tac0turtle Julien mentions to put them in the godocs, which has been updated here: https://github.com/cosmos/cosmos-sdk/pull/18063/files#diff-dfe56fc35673357926b86baaac4cedf6b197fea52180ec4710437bcef5c24e1bR18 Do you mean something in addition to this? |
a small addition to the documentation which would be hosted on our page in order to show this feature exsits |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 70-70: The changelog entry is clear and provides a good summary of the changes made in the PR. It includes the issue number for easy tracking and describes the changes in a user-friendly manner.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- version/version_test.go (3 hunks)
Additional comments: 1
version/version_test.go (1)
- 158-178: The test case for the new feature is well written. It checks that the
ExtraInfo
is correctly retrieved from the context and that the value of a specific key in theExtraInfo
is as expected. It also checks that the command execution does not result in an error and that the output is as expected.
@@ -1,13 +1,13 @@ | |||
package version_test | |||
|
|||
import ( | |||
context "context" |
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.
The import of the context
package is redundant as it's already included in the standard library and doesn't need to be aliased. You can directly use context.Background()
or context.WithValue()
without importing it explicitly.
- context "context"
Commitable suggestion (Beta)
context "context" | |
# context package is already included in the standard library | |
# You can directly use context.Background() or context.WithValue() without importing it explicitly. | |
Co-authored-by: Marko <marbar3778@yahoo.com>
@Mergifyio backport release/v0.50.x |
✅ Backports have been created
|
Co-authored-by: Marko <marbar3778@yahoo.com> (cherry picked from commit 88b7666) # Conflicts: # CHANGELOG.md
Description
Allow apps to provide extra information to version command through ExtraInfo field
Closes: #11690
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
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.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features:
ExtraInfo
feature. This allows the software to store and retrieve additional information provided by apps, improving the overall user experience by providing more context-specific data.Tests:
ExtraInfo
feature, ensuring that the software reliably handles the additional information.This update increases the software's versatility and adaptability, making it more valuable for users who require a more context-aware application.