Provide global output flag for all cli commands #1021
Conversation
|
|
WalkthroughThe changes introduce a global command-line flag for output format selection across various commands in the Yorkie CLI, allowing users to choose between 'yaml' and 'json'. New functions are implemented for output handling in multiple files, centralizing the logic for formatting and printing data. The previous validation function for output options has been removed, simplifying the output handling process. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
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
CodeRabbit Configuration 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: 6
🧹 Outside diff range and nitpick comments (12)
cmd/yorkie/commands.go (2)
59-61: LGTM: Addition of global output flagThe implementation of the global output flag aligns perfectly with the PR objectives. This change enhances the CLI's usability by allowing users to specify the output format consistently across all commands.
Consider adding a default value to the flag definition for better user experience. For example:
-rootCmd.PersistentFlags().StringP("output", "o", "", "One of 'yaml' or 'json'.") +rootCmd.PersistentFlags().StringP("output", "o", "yaml", "Output format: 'yaml' or 'json'")This sets 'yaml' as the default format and slightly improves the help text.
68-74: LGTM with suggestions: Implementation of validateOutputOpts()The validateOutputOpts() function correctly implements the validation logic for the output flag. This ensures that only valid output formats are accepted, which is crucial for the proper functioning of the CLI.
Consider the following improvements:
- As per the static analysis hint, create constants for "yaml" and "json" to improve maintainability:
const ( outputFormatYAML = "yaml" outputFormatJSON = "json" )
- Use these constants in the validation logic and error message:
func validateOutputOpts() error { output := viper.GetString("output") if output != "" && output != outputFormatYAML && output != outputFormatJSON { return fmt.Errorf("--output must be '%s' or '%s'", outputFormatYAML, outputFormatJSON) } return nil }
- Consider using a slice of valid formats for easier maintenance:
var validOutputFormats = []string{outputFormatYAML, outputFormatJSON} func validateOutputOpts() error { output := viper.GetString("output") if output != "" && !contains(validOutputFormats, output) { return fmt.Errorf("--output must be one of: %s", strings.Join(validOutputFormats, ", ")) } return nil } func contains(slice []string, item string) bool { for _, s := range slice { if s == item { return true } } return false }These changes will make the code more maintainable and slightly more efficient.
🧰 Tools
🪛 GitHub Check: build
[failure] 71-71:
stringyamlhas 3 occurrences, make it a constant (goconst)cmd/yorkie/project/create.go (3)
76-78: LGTM: Global output flag implemented correctly.The implementation of the global output flag for the create command is well done. It correctly retrieves the output format from the global configuration and uses the new
printCreateProjectInfofunction to handle the formatting.Consider adding a comment explaining the default behavior when no output format is specified. This would improve code readability and maintainability.
+ // If no output format is specified, it defaults to JSON output := viper.GetString("output")
86-105: LGTM: Output formatting function implemented correctly.The
printCreateProjectInfofunction is well-implemented and supports both JSON and YAML output formats as required. The error handling is appropriate, and the function aligns well with the PR objectives.Consider the following improvements:
- Define constants for the output format strings to improve maintainability and reduce the likelihood of typos:
const ( outputFormatJSON = "json" outputFormatYAML = "yaml" )
- Use these constants in the switch statement and default case:
switch output { case outputFormatJSON, "": // JSON handling case outputFormatYAML: // YAML handling default: return fmt.Errorf("unknown output format: %s", output) }
- Consider using
fmt.Errorffor more informative error messages.These changes will make the code more maintainable and align with Go best practices.
🧰 Tools
🪛 GitHub Check: build
[failure] 88-88:
stringjsonhas 3 occurrences, make it a constant (goconst)
[failure] 94-94:
stringyamlhas 3 occurrences, make it a constant (goconst)
Line range hint
1-106: Overall implementation aligns well with PR objectives.The changes in this file successfully implement the global output flag for the
createcommand, supporting both JSON and YAML formats as required. This implementation aligns perfectly with the PR objectives of providing a consistent method for users to set the output format across CLI commands.Key points:
- The global output flag is correctly retrieved from the viper configuration.
- The new
printCreateProjectInfofunction handles the output formatting based on the specified format.- Error handling is appropriate throughout the implementation.
These changes will significantly improve the usability of the CLI tool, allowing users to easily integrate it into their system workflows with consistent output formats.
To further improve the implementation:
- Consider extracting the output formatting logic into a separate package (e.g.,
outputformatter) that can be reused across different commands. This would promote code reuse and ensure consistency across the CLI tool.- In the future, if more output formats are added, consider using a strategy pattern or a map of formatting functions to make the code more extensible.
🧰 Tools
🪛 GitHub Check: build
[failure] 88-88:
stringjsonhas 3 occurrences, make it a constant (goconst)
[failure] 94-94:
stringyamlhas 3 occurrences, make it a constant (goconst)cmd/yorkie/project/list.go (2)
63-66: LGTM: Changes align with PR objectives. Consider renaming error variable.The modifications to
newListCommandcorrectly implement the global output flag functionality. The error handling is appropriate.Consider renaming
err2to a more descriptive name, such asprintErr, to improve code readability:-err2 := printProjects(cmd, output, projects) -if err2 != nil { - return err2 +printErr := printProjects(cmd, output, projects) +if printErr != nil { + return printErr
74-121: LGTM: Well-implemented output formatting. Consider improving error handling and default case.The
printProjectsfunction effectively centralizes output formatting logic and supports multiple formats as per the PR objectives. Here are some suggestions for improvement:
- Enhance error messages for JSON and YAML marshaling:
-return errors.New("marshal JSON") +return fmt.Errorf("failed to marshal projects to JSON: %w", err) -return errors.New("marshal YAML") +return fmt.Errorf("failed to marshal projects to YAML: %w", err)
- Explicitly handle the default case for output format:
switch output { -case "": +case "", "table": // ... (table formatting logic) case "json": // ... (JSON formatting logic) case "yaml": // ... (YAML formatting logic) default: - return errors.New("unknown output format") + return fmt.Errorf("unknown output format: %s", output) }These changes will improve error clarity and make the default behavior more explicit.
cmd/yorkie/history.go (1)
77-81: LGTM: Improved modularity withprintHistoriesfunctionThe changes effectively implement the global output flag functionality and improve code modularity. The error handling for the new function call is appropriate.
A minor suggestion for improvement:
Consider wrapping the error returned from
printHistoriesto provide more context:if err := printHistories(cmd, output, changes); err != nil { - return err + return fmt.Errorf("failed to print histories: %w", err) }This change would make debugging easier by providing more context about where the error occurred.
cmd/yorkie/document/list.go (3)
84-129: LGTM: Well-implementedprintDocumentsfunction with a minor suggestion.The
printDocumentsfunction is well-structured and correctly implements the different output formats as per the PR objectives. It handles default (table), JSON, and YAML outputs appropriately.Consider using constants for the output format strings to improve maintainability:
const ( outputFormatJSON = "json" outputFormatYAML = "yaml" )Then use these constants in the switch statement:
switch outputFormat { case "": // ... (table output) case outputFormatJSON: // ... (JSON output) case outputFormatYAML: // ... (YAML output) default: // ... }This change would make it easier to maintain and extend the supported output formats in the future.
112-126: LGTM: Correct implementation of JSON and YAML outputs with a suggestion for improvement.The JSON and YAML output implementations are correct and align with the PR objectives. They use appropriate marshaling functions and include error handling.
Consider improving the error messages to be more specific:
case "json": jsonOutput, err := json.MarshalIndent(documents, "", " ") if err != nil { return fmt.Errorf("failed to marshal documents to JSON: %w", err) } cmd.Println(string(jsonOutput)) case "yaml": yamlOutput, err := yaml.Marshal(documents) if err != nil { return fmt.Errorf("failed to marshal documents to YAML: %w", err) } cmd.Println(string(yamlOutput)) default: return fmt.Errorf("unknown output format: %s", outputFormat)These more detailed error messages would make debugging easier if issues arise.
Line range hint
1-130: Overall assessment: Well-implemented global output flag functionality.The changes in this file successfully implement the global output flag for the
document lscommand, aligning well with the PR objectives. The code is well-structured, maintains consistency with existing patterns, and provides appropriate error handling.Key points:
- The new
printDocumentsfunction effectively centralizes output formatting logic.- The implementation supports default (table), JSON, and YAML output formats.
- Error handling is implemented throughout, with some room for improvement in error messages.
- The changes integrate smoothly with the existing codebase.
To further improve the implementation:
- Consider extracting the output formatting logic into a separate package that can be reused across different commands. This would promote code reuse and ensure consistency in output handling across the CLI.
- Implement unit tests for the
printDocumentsfunction to ensure correct behavior for different output formats and edge cases.cmd/yorkie/project/update.go (1)
121-140: LGTM: Well-implemented output formatting functionThe
printUpdateProjectInfofunction effectively implements the PR objectives by supporting both JSON and YAML output formats. The logic is clear and well-structured using a switch statement. Error handling is properly implemented for marshaling errors and unknown output formats.Consider using a constant for the default output format (currently "json") to improve maintainability. For example:
const defaultOutputFormat = "json" // In the switch statement: case defaultOutputFormat, "": // JSON marshaling logicThis would make it easier to change the default format in the future if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- cmd/yorkie/commands.go (2 hunks)
- cmd/yorkie/context/list.go (3 hunks)
- cmd/yorkie/document/list.go (2 hunks)
- cmd/yorkie/history.go (2 hunks)
- cmd/yorkie/project/create.go (2 hunks)
- cmd/yorkie/project/list.go (3 hunks)
- cmd/yorkie/project/update.go (2 hunks)
- cmd/yorkie/version.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
cmd/yorkie/commands.go
[failure] 71-71:
stringyamlhas 3 occurrences, make it a constant (goconst)cmd/yorkie/project/create.go
[failure] 88-88:
stringjsonhas 3 occurrences, make it a constant (goconst)
[failure] 94-94:
stringyamlhas 3 occurrences, make it a constant (goconst)
🔇 Additional comments (16)
cmd/yorkie/commands.go (3)
21-21: LGTM: Import of "errors" packageThe addition of the "errors" import is necessary for the new validateOutputOpts function. This change is appropriate and aligns with the PR objectives.
63-66: LGTM: Addition of PersistentPreRunE for output validationThe implementation of PersistentPreRunE to validate the output option before executing any command is a good practice. This ensures consistent validation across all CLI commands, aligning with the PR objectives.
Line range hint
1-74: Overall assessment: Well-implemented global output flagThe changes in this file successfully implement the global output flag for the CLI, addressing the objectives outlined in the PR and the linked issue #955. The implementation is clean, follows best practices, and enhances the CLI's usability.
Key points:
- The global output flag is correctly added and bound to Viper.
- Validation of the output option is implemented and applied consistently across all commands.
- The code is well-structured and easy to understand.
The suggested improvements, if implemented, will further enhance the code's maintainability and robustness. Great job on this implementation!
🧰 Tools
🪛 GitHub Check: build
[failure] 71-71:
stringyamlhas 3 occurrences, make it a constant (goconst)cmd/yorkie/project/create.go (1)
28-31: LGTM: New imports align with PR objectives.The addition of the
yaml.v3andtypesimports are appropriate for implementing the new YAML output format and handling the Project type. These changes align well with the PR objectives of providing a global output flag for CLI commands.cmd/yorkie/project/list.go (2)
21-22: LGTM: New imports are appropriate for the added functionality.The added imports for JSON, YAML, errors, and types are necessary and correctly implemented for the new output formatting features.
Also applies to: 28-28, 31-31
Line range hint
1-122: Summary: Implementation successfully meets PR objectives.The changes in this file effectively implement the global output flag functionality for the
project lscommand, aligning well with the PR objectives. The newprintProjectsfunction centralizes the output formatting logic, supporting table, JSON, and YAML formats as required. This enhancement improves the CLI's flexibility and user experience.To verify the implementation against the PR objectives:
These tests will confirm that the global output flag is properly implemented, supports the required output formats, and handles errors for unknown formats.
✅ Verification successful
Verification Successful
The implementation of the global output flag for the
project lscommand has been successfully verified. The code includes theviper.GetString("output")call, supportsjson,yaml, and default output formats, and properly handles unknown output formats by returning an error.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the global output flag for the project ls command. # Test 1: Check if the output flag is properly added to the command rg --type go 'viper\.GetString\("output"\)' cmd/yorkie/project/list.go # Test 2: Verify support for different output formats rg --type go 'case "json":|case "yaml":|case "":' cmd/yorkie/project/list.go # Test 3: Ensure error handling for unknown output formats rg --type go 'return errors\.New\("unknown output format"\)' cmd/yorkie/project/list.goLength of output: 363
cmd/yorkie/history.go (3)
21-21: LGTM: New imports added for JSON and YAML supportThe new imports for JSON and YAML are appropriate for the added functionality of outputting in these formats. The addition of the
typesimport is also correct, as it's needed for theChangeSummarytype used in the newprintHistoriesfunction.Also applies to: 27-27, 30-30
Line range hint
1-150: Overall implementation looks good, with some suggestions for improvementThe changes effectively implement the functionality for different output formats as required by the PR objectives. The code is well-structured and modular, with the new
printHistoriesfunction centralizing the output logic.Summary of main points:
- The new imports and changes in the
newHistoryCmdfunction are appropriate.- The
printHistoriesfunction is well-implemented but could benefit from more specific error messages.- Consider adding explicit handling for unexpected output format values.
- The absence of a visible global output flag in this file is a concern. Please verify if it's added elsewhere in the codebase.
These changes significantly improve the CLI tool's usability by providing consistent output formatting across commands. Once the global output flag is confirmed or added, this implementation will fully meet the PR objectives.
Line range hint
128-150: Consider adding a global output flagThe
initfunction remains unchanged. Given the PR objectives to implement a global output flag, we might expect to see the addition of this flag here.Could you verify if the global output flag is added elsewhere in the codebase? If not, consider adding it here:
func init() { cmd := newHistoryCmd() + cmd.Flags().StringVar( + &output, + "output", + "", + "Output format: '', 'json', or 'yaml'", + ) // ... existing flags ... rootCmd.AddCommand(cmd) }Run the following script to check for the presence of a global output flag:
cmd/yorkie/document/list.go (3)
21-21: LGTM: New imports for JSON and YAML handling.The addition of
encoding/jsonandgopkg.in/yaml.v3imports is appropriate for implementing the new output formats.Also applies to: 28-28
74-78: LGTM: Implementation of global output flag innewListCommand.The changes correctly implement the global output flag functionality:
- Retrieves the output format from viper.
- Calls the new
printDocumentsfunction with the appropriate parameters.- Implements proper error handling.
This implementation aligns well with the PR objectives.
87-111: LGTM: Well-implemented table output.The table output implementation:
- Uses the
go-prettylibrary effectively.- Sets appropriate style options for a clean, compact output.
- Includes all relevant document information.
- Uses
units.HumanDurationfor improved readability of time-related data.This implementation maintains consistency with the previous output while integrating well with the new global output flag functionality.
cmd/yorkie/version.go (1)
Line range hint
27-53: Update command help text for global output flagThe command implementation looks good, but the help text for the command should be updated to reflect the use of a global output flag.
Consider updating the command's
Shortdescription or adding aLongdescription to mention the global output flag:Short: "Print the version number of Yorkie", Long: `Print the version number of Yorkie. This command supports the global output flag for formatting the result.`,Let's verify if other commands in the CLI have been updated to use the global output flag:
#!/bin/bash # Search for usage of viper.GetString("output") in other command files rg --type go 'viper.GetString\("output"\)' -g 'cmd/yorkie/*.go' -g '!cmd/yorkie/version.go'cmd/yorkie/project/update.go (3)
28-28: LGTM: YAML package import addedThe addition of the
gopkg.in/yaml.v3package is consistent with the PR objectives to support YAML output format. This import is correctly placed and follows Go conventions.
111-113: LGTM: Consistent output formatting implementedThe replacement of direct JSON marshaling with a call to
printUpdateProjectInfoaligns well with the PR objectives. It introduces a consistent method for output formatting across commands. Theoutputvariable is retrieved from viper, suggesting it's configurable, which enhances flexibility. Proper error handling is also implemented.
Line range hint
1-141: Overall implementation looks good, but verify global applicationThe changes successfully implement support for a global output flag in the project update command, addressing the main objectives of the PR. The implementation is clean, follows Go best practices, and provides comprehensive error handling.
However, to fully satisfy the objective of providing a "global output flag for all CLI commands", we should verify that similar changes have been made to other relevant commands (e.g.,
context ls,project create,project ls,document ls,history ls).To ensure consistency across all commands, please run the following script:
If the script doesn't find similar implementations in other command files, consider applying this pattern to those commands as well to achieve a truly global output flag.
✅ Verification successful
Global output flag implementation verified across all relevant CLI commands
The shell script results confirm that the global output flag has been implemented in all other relevant commands, ensuring consistency and fulfilling the objectives of the PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other commands implement the global output flag # Test: Search for the implementation of output formatting in other command files rg --type go -g 'cmd/yorkie/**/*.go' -e 'viper\.GetString\("output"\)' -e 'printUpdateProjectInfo' # Expected result: Similar implementations in other command filesLength of output: 839
cmd/yorkie/history.go
Outdated
| func printHistories(cmd *cobra.Command, output string, changes []*types.ChangeSummary) error { | ||
| switch output { | ||
| case "": | ||
| tw := table.NewWriter() | ||
| tw.Style().Options.DrawBorder = false | ||
| tw.Style().Options.SeparateColumns = false | ||
| tw.Style().Options.SeparateFooter = false | ||
| tw.Style().Options.SeparateHeader = false | ||
| tw.Style().Options.SeparateRows = false | ||
| tw.AppendHeader(table.Row{ | ||
| "SEQ", | ||
| "MESSAGE", | ||
| "SNAPSHOT", | ||
| }) | ||
| for _, change := range changes { | ||
| tw.AppendRow(table.Row{ | ||
| change.ID.ServerSeq(), | ||
| change.Message, | ||
| change.Snapshot, | ||
| }) | ||
| } | ||
| cmd.Printf("%s\n", tw.Render()) | ||
| case "json": | ||
| jsonOutput, err := json.MarshalIndent(changes, "", " ") | ||
| if err != nil { | ||
| return errors.New("marshal JSON") | ||
| } | ||
| cmd.Println(string(jsonOutput)) | ||
| case "yaml": | ||
| yamlOutput, err := yaml.Marshal(changes) | ||
| if err != nil { | ||
| return errors.New("marshal YAML") | ||
| } | ||
| cmd.Println(string(yamlOutput)) | ||
| default: | ||
| return errors.New("unknown output format") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Well-implemented printHistories function with suggestions for improvement
The printHistories function effectively handles different output formats as required. The implementation is clean and modular.
Suggestions for improvement:
- Enhance error messages for JSON and YAML marshaling:
if err != nil {
- return errors.New("marshal JSON")
+ return fmt.Errorf("failed to marshal JSON: %w", err)
} if err != nil {
- return errors.New("marshal YAML")
+ return fmt.Errorf("failed to marshal YAML: %w", err)
}- Handle unexpected output format values explicitly:
default:
- return errors.New("unknown output format")
+ return fmt.Errorf("unknown output format: %s", output)These changes will provide more informative error messages, making debugging easier for users and developers.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func printHistories(cmd *cobra.Command, output string, changes []*types.ChangeSummary) error { | |
| switch output { | |
| case "": | |
| tw := table.NewWriter() | |
| tw.Style().Options.DrawBorder = false | |
| tw.Style().Options.SeparateColumns = false | |
| tw.Style().Options.SeparateFooter = false | |
| tw.Style().Options.SeparateHeader = false | |
| tw.Style().Options.SeparateRows = false | |
| tw.AppendHeader(table.Row{ | |
| "SEQ", | |
| "MESSAGE", | |
| "SNAPSHOT", | |
| }) | |
| for _, change := range changes { | |
| tw.AppendRow(table.Row{ | |
| change.ID.ServerSeq(), | |
| change.Message, | |
| change.Snapshot, | |
| }) | |
| } | |
| cmd.Printf("%s\n", tw.Render()) | |
| case "json": | |
| jsonOutput, err := json.MarshalIndent(changes, "", " ") | |
| if err != nil { | |
| return errors.New("marshal JSON") | |
| } | |
| cmd.Println(string(jsonOutput)) | |
| case "yaml": | |
| yamlOutput, err := yaml.Marshal(changes) | |
| if err != nil { | |
| return errors.New("marshal YAML") | |
| } | |
| cmd.Println(string(yamlOutput)) | |
| default: | |
| return errors.New("unknown output format") | |
| } | |
| return nil | |
| } | |
| func printHistories(cmd *cobra.Command, output string, changes []*types.ChangeSummary) error { | |
| switch output { | |
| case "": | |
| tw := table.NewWriter() | |
| tw.Style().Options.DrawBorder = false | |
| tw.Style().Options.SeparateColumns = false | |
| tw.Style().Options.SeparateFooter = false | |
| tw.Style().Options.SeparateHeader = false | |
| tw.Style().Options.SeparateRows = false | |
| tw.AppendHeader(table.Row{ | |
| "SEQ", | |
| "MESSAGE", | |
| "SNAPSHOT", | |
| }) | |
| for _, change := range changes { | |
| tw.AppendRow(table.Row{ | |
| change.ID.ServerSeq(), | |
| change.Message, | |
| change.Snapshot, | |
| }) | |
| } | |
| cmd.Printf("%s\n", tw.Render()) | |
| case "json": | |
| jsonOutput, err := json.MarshalIndent(changes, "", " ") | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal JSON: %w", err) | |
| } | |
| cmd.Println(string(jsonOutput)) | |
| case "yaml": | |
| yamlOutput, err := yaml.Marshal(changes) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal YAML: %w", err) | |
| } | |
| cmd.Println(string(yamlOutput)) | |
| default: | |
| return fmt.Errorf("unknown output format: %s", output) | |
| } | |
| return nil | |
| } |
cmd/yorkie/version.go
Outdated
| @@ -57,6 +53,7 @@ func newVersionCmd() *cobra.Command { | |||
| info.ServerVersion, serverErr = fetchServerVersion() | |||
| } | |||
|
|
|||
| output = viper.GetString("output") | |||
There was a problem hiding this comment.
💡 Codebase verification
Implement Consistent Error Handling for output Flag
Validation for the output flag is not consistently handled across all commands. Ensure that each command validates the output value to prevent invalid formats.
🔗 Analysis chain
Approve change, but consider adding error handling
The change aligns well with the PR objective of introducing a global output flag. Using viper.GetString("output") allows for a consistent approach across all commands.
However, consider adding error handling for invalid output formats at this point. While the validation might be handled globally, it's good practice to have a fallback here.
Consider adding error handling:
output = viper.GetString("output")
if output != "" && output != "yaml" && output != "json" {
return fmt.Errorf("invalid output format: %s", output)
}Let's verify the global implementation of output flag validation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for global output flag validation
rg --type go 'func validateOutputOpts|viper.GetString\("output"\)' -g '!cmd/yorkie/version.go'
Length of output: 214
cmd/yorkie/context/list.go
Outdated
| return errors.New("marshal JSON") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include Original Error When Marshaling JSON Fails
Wrapping the original error provides more context for debugging marshaling issues.
Update the error handling to include the original error:
- return errors.New("marshal JSON")
+ return fmt.Errorf("failed to marshal JSON: %w", err)Remember to import "fmt" at the beginning of the file if it's not already imported.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return errors.New("marshal JSON") | |
| } | |
| return fmt.Errorf("failed to marshal JSON: %w", err) | |
| } |
cmd/yorkie/context/list.go
Outdated
| return errors.New("unknown output format") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide More Informative Error for Unknown Output Format
Including the invalid outputFormat in the error message helps users understand the issue.
Modify the error message to include the provided output format:
- return errors.New("unknown output format")
+ return fmt.Errorf("unknown output format: %s", outputFormat)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return errors.New("unknown output format") | |
| } | |
| return fmt.Errorf("unknown output format: %s", outputFormat) | |
| } |
cmd/yorkie/context/list.go
Outdated
| return errors.New("marshal YAML") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include Original Error When Marshaling YAML Fails
Similarly, include the original error for YAML marshaling failures.
Adjust the error return statement:
- return errors.New("marshal YAML")
+ return fmt.Errorf("failed to marshal YAML: %w", err)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return errors.New("marshal YAML") | |
| } | |
| return fmt.Errorf("failed to marshal YAML: %w", err) | |
| } |
| type contextInfo struct { | ||
| Current string `json:"current" yaml:"current"` | ||
| RPCAddr string `json:"rpc_addr" yaml:"rpc_addr"` | ||
| Insecure string `json:"insecure" yaml:"insecure"` | ||
| Token string `json:"token" yaml:"token"` | ||
| } |
There was a problem hiding this comment.
Security Concern: Exposing Full Token in JSON and YAML Outputs
Including the full Token in the JSON and YAML outputs may inadvertently expose sensitive authentication information, posing a security risk if the output is logged or shared.
Consider obfuscating or excluding the Token field in the JSON and YAML outputs to protect sensitive data.
You can modify the contextInfo struct to omit the Token field during marshaling:
type contextInfo struct {
Current string `json:"current" yaml:"current"`
RPCAddr string `json:"rpc_addr" yaml:"rpc_addr"`
Insecure string `json:"insecure" yaml:"insecure"`
- Token string `json:"token" yaml:"token"`
+ Token string `json:"-"`
// Use ObfuscatedToken for display purposes
+ ObfuscatedToken string `json:"token" yaml:"token"`
}Then, when appending to contexts, set ObfuscatedToken with the obfuscated token:
contexts = append(contexts, contextInfo{
Current: current,
RPCAddr: rpcAddr,
Insecure: insecure,
- Token: ellipsisToken,
+ Token: auth.Token, // This will be ignored during marshaling
+ ObfuscatedToken: ellipsisToken,
})📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type contextInfo struct { | |
| Current string `json:"current" yaml:"current"` | |
| RPCAddr string `json:"rpc_addr" yaml:"rpc_addr"` | |
| Insecure string `json:"insecure" yaml:"insecure"` | |
| Token string `json:"token" yaml:"token"` | |
| } | |
| type contextInfo struct { | |
| Current string `json:"current" yaml:"current"` | |
| RPCAddr string `json:"rpc_addr" yaml:"rpc_addr"` | |
| Insecure string `json:"insecure" yaml:"insecure"` | |
| Token string `json:"-"` | |
| ObfuscatedToken string `json:"token" yaml:"token"` | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
cmd/yorkie/output.go (1)
1-7: LGTM! The file structure and constants align with the PR objectives.The new
output.gofile successfully introduces constants for different output formats, which can be used globally across CLI commands as intended. The implementation includes JSON and YAML options as mentioned in the PR summary, and these constants can be used to validate user input for output formats.A few suggestions for potential improvements:
- Consider adding a package-level comment to explain the purpose of this file.
- The use of an empty string for
DefaultOutputmight be implicit. Consider using a more explicit value like "table".- For better type safety, you could use an enum-like structure using custom types.
Here's an example of how you could implement these suggestions:
// Package main provides constants and utilities for CLI output formatting. package main type OutputFormat string const ( DefaultOutput OutputFormat = "table" // DefaultOutput is for table format YamlOutput OutputFormat = "yaml" // YamlOutput is for yaml format JSONOutput OutputFormat = "json" // JSONOutput is for json format )This approach provides better type safety and makes the default format more explicit.
cmd/yorkie/project/project.go (1)
30-34: LGTM! Consider using iota for enum-like constants.The addition of these constants aligns well with the PR objectives of providing a global output flag for all CLI commands. The naming and comments are clear and follow Go best practices.
Consider using
iotafor these constants to make them more enum-like and easier to maintain:const ( DefaultOutput = iota // DefaultOutput is for table format YamlOutput // YamlOutput is for yaml format JSONOutput // JSONOutput is for json format ) var OutputFormatStrings = map[int]string{ DefaultOutput: "", YamlOutput: "yaml", JSONOutput: "json", }This approach allows for easier addition of new output formats in the future and provides a clearer representation of the constants as a related set.
cmd/yorkie/version.go (1)
57-57: Consider adding error handling for invalid output formats.The change aligns well with the PR objective of introducing a global output flag. Using
viper.GetString("output")allows for a consistent approach across all commands.However, consider adding error handling for invalid output formats at this point. While the validation might be handled globally, it's good practice to have a fallback here.
Consider adding error handling:
output := viper.GetString("output") if output != "" && output != "yaml" && output != "json" { return fmt.Errorf("invalid output format: %s", output) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- cmd/yorkie/commands.go (2 hunks)
- cmd/yorkie/context/list.go (3 hunks)
- cmd/yorkie/document/list.go (2 hunks)
- cmd/yorkie/history.go (2 hunks)
- cmd/yorkie/output.go (1 hunks)
- cmd/yorkie/project/create.go (2 hunks)
- cmd/yorkie/project/list.go (3 hunks)
- cmd/yorkie/project/project.go (1 hunks)
- cmd/yorkie/project/update.go (2 hunks)
- cmd/yorkie/version.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/yorkie/commands.go
- cmd/yorkie/context/list.go
- cmd/yorkie/document/list.go
- cmd/yorkie/history.go
- cmd/yorkie/project/create.go
- cmd/yorkie/project/list.go
- cmd/yorkie/project/update.go
🔇 Additional comments (5)
cmd/yorkie/version.go (5)
23-23: LGTM: Import addition is appropriate.The addition of the "fmt" package aligns with the improvements in error handling and output formatting mentioned in the PR objectives.
129-132: LGTM: Improved YAML output handling.The use of the
YamlOutputconstant and the enhanced error message for YAML marshaling are good improvements. These changes enhance code readability and provide better context for debugging.
135-138: LGTM: Improved JSON output handling.The use of the
JSONOutputconstant and the enhanced error message for JSON marshaling are good improvements. These changes are consistent with the YAML output case and enhance both code readability and debugging context.
142-142: LGTM: Improved error handling for unknown output formats.The addition of a default case to handle unknown output formats is a good improvement. It provides clear and informative error messages, enhancing the CLI tool's usability and aligning with the PR objectives.
Line range hint
1-158: Overall, the changes align well with the PR objectives and improve code quality.The modifications to
cmd/yorkie/version.gosuccessfully implement the global output flag and enhance error handling. The use of constants for output formats and improved error messages contribute to better code readability and maintainability.Consider implementing the suggested error handling for invalid output formats in the
newVersionCmdfunction to further improve robustness.These changes effectively address the requirements outlined in issue #955 and enhance the overall usability of the CLI tool.
| const ( | ||
| DefaultOutput = "" // DefaultOutput is for table format | ||
| YamlOutput = "yaml" // YamlOutput is for yaml format | ||
| JSONOutput = "json" // JSONOutput is for json format | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Hardcoded output format strings detected.
The following locations still use hardcoded "yaml" or "json" strings instead of the defined constants:
-
cmd/yorkie/commands.go:viper.SetConfigType("json")
-
cmd/yorkie/document/list.go:case "json":case "yaml":
-
cmd/yorkie/context/list.go:case "json":case "yaml":
Please replace these hardcoded strings with the appropriate constants to ensure consistency across the codebase.
🔗 Analysis chain
Verify consistent usage of output format constants.
While the addition of these constants is a good start for implementing the global output flag functionality, it's important to ensure they are used consistently across all relevant CLI commands.
Let's verify the usage of these constants across the codebase:
Please review the results to ensure that:
- These constants are used in all relevant CLI command implementations.
- There are no remaining hardcoded "yaml" or "json" strings for output formats.
- The constants are used consistently across different commands.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of output format constants across the codebase.
echo "Checking usage of DefaultOutput:"
rg --type go "DefaultOutput"
echo "\nChecking usage of YamlOutput:"
rg --type go "YamlOutput"
echo "\nChecking usage of JSONOutput:"
rg --type go "JSONOutput"
echo "\nChecking for any hardcoded output format strings:"
rg --type go '"yaml"|"json"'
Length of output: 2525
What this PR does / why we need it:
Provide a consistent and convenient way for users to set the output format for all CLI commands
Add
jsonandyamldata format as options of output flag, changed it to globally.Validation for options that are input by user was also changed as globally. ebbf4f0
Following prior work, implemented output format for each commands below
context lsproject createproject lsproject updatedocument lshistory lsWhich issue(s) this PR fixes:
Fixes #955
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor