block
fix: shows the correct config file update path with cli configure
Signed-off-by: Anthony D. Mays [email protected]
Summary
goose configure will now show the correct config files updated after applying changes. Updates the initial message to suggest that users can change the configs in ~/.config/goose
Note: This is my first time coding in Rust! 😝
Type of Change
- [ ] Feature
- [x] Bug fix
- [ ] Refactor / Code quality
- [ ] Performance improvement
- [ ] Documentation
- [ ] Tests
- [ ] Security fix
- [ ] Build / Release
- [ ] Other (specify below)
Testing
Manual testing
Related Issues
Relates to #5117 , closes #5196
hi @DOsinga, @michaelneale, and @alexhancock -- this is for your team to review. It's a hacktoberfest PR, but it touches Rust and the CLI tooling.
the copilot suggestions actually seem quite reasonable to follow up. I would delete some of the comments. also, question, why not print the name of the config file rather than the folder? I know we do store some other stuff there, but I don't think that is touched here
the copilot suggestions actually seem quite reasonable to follow up. I would delete some of the comments. also, question, why not print the name of the config file rather than the folder? I know we do store some other stuff there, but I don't think that is touched here
Will follow-up on the copilot suggestions.
Re: printing file name, we will continue to do so. This change results in us printing the name of the modified config file after making changes with goose configure. This is usually just config.yaml but can also be permission.yaml for tool permissions.
As for the folder name, we'll only show it once in the initial message displayed by goose configure so that the user understands that there are other config files present in that directory. Accompanying docs changes are in #5210 to clarify that multiple files are involved in goose config, not just config.yaml.
Noticed that test_check_tool_permissions_smart_approve was failing on PR check, made a small change to see if it now will pass (does locally on my machine). Not sure if it's just flakey though.
Hi @DOsinga @alexhancock @michaelneale @jamadeo @zanesq , following up with a ping for review as we approach the end of Hacktoberfest 🙏
Thanks for the LGTM!
In handle_configure() you now print the config directory (Paths::config_dir()), but in many places you print the config file path (config.path()). This leads to inconsistent messages ("edit them directly at <config_dir>" vs "Configuration saved successfully to <config.path()>"). Decide whether you want to display the config file path or the config directory and standardize messages accordingly. In print_config_file_saved() the wording is singular ("Configuration saved successfully to {}") but earlier you changed help text to refer to "config files" (plural). Consider harmonizing the phrasing.
We only now show the config dir in two places where, in context, it's more correct to show it since it's referring to the location of all config related files (at the start of goose configure and in goose info). Otherwise, we want to show the specific file that was modified (per #5117) .
Some functions (e.g. configure_custom_provider_dialog -> add_provider/remove_provider) may already call cliclack::outro or other success messages. You added print_config_file_saved() after the match in configure_custom_provider_dialog and in several other places. Verify you won't produce duplicate success messages. Stylistic / minor suggestions
Verified that we don't get duplicate success messages
Path -> String conversions are inconsistent: You use to_string_lossy().to_string() in handle_configure, .display().to_string() in other places, and sometimes just pass a Path to format! which works because Display is implemented. Prefer one consistent approach (e.g., config.path().display().to_string() or config.path().to_string_lossy()). In configure_provider_dialog you log: let _ = cliclack::log::info(format!("Saved {} to {}", key.name, config.path())); That works, but consider formatting the path consistently (call .display() or .to_string_lossy()) so logging is uniform across platforms.
Swapped to_string_lossy() for display() in handle_configure. As for the other occurrences, there are a lot of places in the code where to_string_lossy is used for path printing and I didn't want to stir that pot in this PR. Happy to file a bug for a broader cleanup effort.
Thank you so much for reviewing @zanesq , and for your work on this contribution @anthonydmays ! After tests run, happy to merge. Hope you've been enjoying Hacktoberfest 🎃 ❤️