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

fix(client/v2): respect output format from client ctx #20033

Merged
merged 5 commits into from
Apr 15, 2024
Merged

fix(client/v2): respect output format from client ctx #20033

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
016bb5e
fix: respect output format from client ctx
mmsqe Apr 13, 2024
3ba2c02
update doc
mmsqe Apr 13, 2024
e1a97d2
Apply suggestions from code review
mmsqe Apr 13, 2024
97c89d1
rm aux
mmsqe Apr 13, 2024
8539ab5
Apply suggestions from code review
mmsqe Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (baseapp) [#19970](https://github.com/cosmos/cosmos-sdk/pull/19970) Fix default config values to use no-op mempool as default.
* (crypto) [#20027](https://github.com/cosmos/cosmos-sdk/pull/20027) secp256r1 keys now implement gogoproto's customtype interface.
* (x/bank) [#20028](https://github.com/cosmos/cosmos-sdk/pull/20028) Align query with multi denoms for send-enabled.
* (cli) [#20033](https://github.com/cosmos/cosmos-sdk/pull/20033) Respect output format from client ctx.
Copy link
Member

Choose a reason for hiding this comment

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

Changelog should be under client/v2


### API Breaking Changes

Expand Down
12 changes: 9 additions & 3 deletions client/v2/autocli/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
"cosmossdk.io/client/v2/internal/flags"
"cosmossdk.io/client/v2/internal/util"

"github.com/cosmos/cosmos-sdk/client"
)

type cmdType int
Expand Down Expand Up @@ -224,11 +226,15 @@ func enhanceCustomCmd(builder *Builder, cmd *cobra.Command, cmdType cmdType, mod

// outOrStdoutFormat formats the output based on the output flag and writes it to the command's output stream.
func (b *Builder) outOrStdoutFormat(cmd *cobra.Command, out []byte) error {
var err error
outputType := cmd.Flag(flags.FlagOutput)
clientCtx, err := client.GetClientTxContext(cmd)
Copy link
Member

Choose a reason for hiding this comment

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

Can we directly get the client context from its context key instead of using the helper and if not present fallback to the current logic? We are trying to make client.Context optional in the AutoCLI core logic (I know we are not there yet 😅, but that change would not add extra dependencies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems still need import client for context and key

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we need to import client context anyway for the type check, but that's okay. It just makes clearer what our deps on client.Context are.

if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling in outOrStdoutFormat.

-	if err != nil {
-		return err
-	}
+	if err != nil {
+		return fmt.Errorf("failed to get client transaction context: %w", err)
+	}

This change enhances the error message, providing more context about where the error occurred, which is helpful for debugging.

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.

Suggested change
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
}
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return fmt.Errorf("failed to get client transaction context: %w", err)
}


outputType := clientCtx.OutputFormat
// if the output type is text, convert the json to yaml
// if output type is json or nil, default to json
if outputType != nil && outputType.Value.String() == flags.OutputFormatText {
if outputType == flags.OutputFormatText {
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
out, err = yaml.JSONToYAML(out)
if err != nil {
return err
Expand Down
Loading