chore!: rename tools-file flags to config#2738
chore!: rename tools-file flags to config#2738Yuan325 wants to merge 1 commit intoremove-deprecationfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the command-line interface for tool configuration by introducing new, more descriptive flag names. The change aims to enhance the user experience by making flag usage more intuitive and consistent across the application. It involves a comprehensive update of the codebase, including internal logic, tests, and all user-facing documentation and examples, ensuring a smooth transition for users while maintaining backward compatibility for a deprecation period. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors command-line flags for configuration files. The review highlights a critical issue in the flag deprecation implementation that needs correction to prevent CLI breakage. Additionally, there's a minor bug in an error message and suggestions for file and test renames to enhance maintainability and consistency. While the refactoring improves flag clarity, addressing these points is crucial for a robust implementation.
| persistentFlags.StringVar(&opts.Config, "config", "", "File path specifying the tool configuration. Cannot be used with --configs, or --config-folder.") | ||
| persistentFlags.StringVar(&opts.Config, "config", "", "File path specifying the tool configuration. Cannot be used with --configs, or --config-folder.") | ||
| _ = persistentFlags.MarkDeprecated("config", "please use --config instead") // DEPRECATED | ||
| persistentFlags.StringSliceVar(&opts.Configs, "configs", []string{}, "Multiple file paths specifying tool configurations. Files will be merged. Cannot be used with --config, or --config-folder.") | ||
| persistentFlags.StringSliceVar(&opts.Configs, "configs", []string{}, "Multiple file paths specifying tool configurations. Files will be merged. Cannot be used with --config, or --config-folder.") | ||
| _ = persistentFlags.MarkDeprecated("configs", "please use --configs instead") // DEPRECATED | ||
| persistentFlags.StringVar(&opts.ConfigFolder, "config-folder", "", "Directory path containing YAML tool configuration files. All .yaml and .yml files in the directory will be loaded and merged. Cannot be used with --config, or --configs.") | ||
| persistentFlags.StringVar(&opts.ConfigFolder, "config-folder", "", "Directory path containing YAML tool configuration files. All .yaml and .yml files in the directory will be loaded and merged. Cannot be used with --config, or --configs.") | ||
| _ = persistentFlags.MarkDeprecated("config-folder", "please use --config-folder instead") // DEPRECATED |
There was a problem hiding this comment.
There appears to be a copy-paste error in the flag definitions. The new flags (config, configs, config-folder) are being defined twice and then immediately marked as deprecated in favor of themselves. This will cause unexpected behavior and prevent the CLI from working as intended.
To correctly deprecate the old flags (tools-file, tools-files, tools-folder) while introducing the new ones, you should define the new flags, and then define the old flags with a MarkDeprecated call, pointing to the same underlying option variable.
| persistentFlags.StringVar(&opts.Config, "config", "", "File path specifying the tool configuration. Cannot be used with --configs, or --config-folder.") | |
| persistentFlags.StringVar(&opts.Config, "config", "", "File path specifying the tool configuration. Cannot be used with --configs, or --config-folder.") | |
| _ = persistentFlags.MarkDeprecated("config", "please use --config instead") // DEPRECATED | |
| persistentFlags.StringSliceVar(&opts.Configs, "configs", []string{}, "Multiple file paths specifying tool configurations. Files will be merged. Cannot be used with --config, or --config-folder.") | |
| persistentFlags.StringSliceVar(&opts.Configs, "configs", []string{}, "Multiple file paths specifying tool configurations. Files will be merged. Cannot be used with --config, or --config-folder.") | |
| _ = persistentFlags.MarkDeprecated("configs", "please use --configs instead") // DEPRECATED | |
| persistentFlags.StringVar(&opts.ConfigFolder, "config-folder", "", "Directory path containing YAML tool configuration files. All .yaml and .yml files in the directory will be loaded and merged. Cannot be used with --config, or --configs.") | |
| persistentFlags.StringVar(&opts.ConfigFolder, "config-folder", "", "Directory path containing YAML tool configuration files. All .yaml and .yml files in the directory will be loaded and merged. Cannot be used with --config, or --configs.") | |
| _ = persistentFlags.MarkDeprecated("config-folder", "please use --config-folder instead") // DEPRECATED | |
| persistentFlags.StringVar(&opts.Config, "config", "", "File path specifying the tool configuration. Cannot be used with --configs, or --config-folder.") | |
| persistentFlags.StringSliceVar(&opts.Configs, "configs", []string{}, "Multiple file paths specifying tool configurations. Files will be merged. Cannot be used with --config, or --config-folder.") | |
| persistentFlags.StringVar(&opts.ConfigFolder, "config-folder", "", "Directory path containing YAML tool configuration files. All .yaml and .yml files in the directory will be loaded and merged. Cannot be used with --config, or --configs.") | |
| // Deprecated flags | |
| persistentFlags.StringVar(&opts.Config, "tools-file", "", "DEPRECATED: use --config instead.") | |
| _ = persistentFlags.MarkDeprecated("tools-file", "please use --config instead") | |
| persistentFlags.StringSliceVar(&opts.Configs, "tools-files", []string{}, "DEPRECATED: use --configs instead.") | |
| _ = persistentFlags.MarkDeprecated("tools-files", "please use --configs instead") | |
| persistentFlags.StringVar(&opts.ConfigFolder, "tools-folder", "", "DEPRECATED: use --config-folder instead.") | |
| _ = persistentFlags.MarkDeprecated("tools-folder", "please use --config-folder instead") |
| if (opts.Config != "" && len(opts.Configs) > 0) || | ||
| (opts.Config != "" && opts.ConfigFolder != "") || | ||
| (len(opts.Configs) > 0 && opts.ConfigFolder != "") { | ||
| errMsg := fmt.Errorf("--config/--config, --configs/--configs, and --config-folder/--config-folder flags cannot be used simultaneously") |
There was a problem hiding this comment.
The error message for mutually exclusive flags appears to be malformed. It currently says --config/--config, --configs/--configs, ... which seems to be a copy-paste error. This will also cause the TestMutuallyExclusiveFlags test to fail.
The message should list the flags separated by commas.
| errMsg := fmt.Errorf("--config/--config, --configs/--configs, and --config-folder/--config-folder flags cannot be used simultaneously") | |
| errMsg := fmt.Errorf("--config, --configs, and --config-folder flags cannot be used simultaneously") |
| ) | ||
|
|
||
| type ToolsFile struct { | ||
| type Config struct { |
There was a problem hiding this comment.
For better maintainability and clarity, consider renaming this file from tools_file.go to config.go. This would align the filename with the Config and ConfigParser types that are now defined within it. This can be done in a follow-up change if it's easier.
| @@ -539,9 +539,9 @@ func TestParseToolFile(t *testing.T) { | |||
| t.Fatalf("unexpected error: %s", err) | |||
There was a problem hiding this comment.
For consistency with the renames in config.go (previously tools_file.go), this test function should be renamed from TestParseToolFile to TestParseConfig.
Additionally, it would improve clarity to rename the test file itself from tools_file_test.go to config_test.go in a follow-up change.
2 participants
tools_fileflag.tools-file,tools-files, andtools-folderflag. Renaming them toconfig,configs, andconfig-folderflag.