-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: shows the correct config file update path with cli configure #5195
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
Conversation
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.
Pull Request Overview
Fixes CLI messaging to display accurate configuration locations and confirm where settings are saved.
- Add PermissionManager::get_config_path and use it to show the saved permission config path.
- Update goose configure prompts/outros to reference the config directory and include explicit save paths.
- Update info command to show the config directory instead of a single config file.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| crates/goose/src/config/permission.rs | Adds get_config_path to expose the permission config file path. |
| crates/goose-cli/src/commands/info.rs | Switches from “Config file” to “Config dir” and uses Paths::config_dir for display. |
| crates/goose-cli/src/commands/configure.rs | Adjusts CLI messages to include save paths; adds print_config_file_saved helper; updates various dialogs to print where configs were saved. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
aeb3466 to
c136997
Compare
|
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. |
c136997 to
37cb541
Compare
|
Also tagging @jamadeo ! |
|
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 As for the folder name, we'll only show it once in the initial message displayed by |
37cb541 to
bee6561
Compare
|
@DOsinga co-pilot comments addressed. Thanks! |
bee6561 to
a931ed7
Compare
|
Noticed that |
|
Friendly ping |
|
Thank you! The dev team will review ASAP 🙏 |
|
Hi @DOsinga @alexhancock @michaelneale @jamadeo @zanesq , following up with a ping for review as we approach the end of Hacktoberfest 🙏 |
zanesq
left a comment
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.
LGTM, copilot suggested some minor issues if you want to take a look
Behavioral / consistency issues
Mixed messages about "config file" vs "config dir":
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.
Possible duplicate/outro messages
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
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.
Signed-off-by: Anthony D. Mays <anthony@morganlatimer.com>
Signed-off-by: Anthony D. Mays <anthony@morganlatimer.com>
Signed-off-by: Anthony D. Mays <anthony@morganlatimer.com>
Signed-off-by: Anthony D. Mays <anthony@morganlatimer.com>
Signed-off-by: Anthony D. Mays <anthony@morganlatimer.com>
a931ed7 to
5407f63
Compare
|
Thanks for the LGTM!
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
Verified that we don't get duplicate success messages
Swapped |
|
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 🎃 ❤️ |
…ock#5195) Signed-off-by: Anthony D. Mays <anthony@morganlatimer.com> Signed-off-by: fbalicchia <fbalicchia@gmail.com>
…ock#5195) Signed-off-by: Anthony D. Mays <anthony@morganlatimer.com> Signed-off-by: Blair Allan <Blairallan@icloud.com>
Signed-off-by: Anthony D. Mays anthony@morganlatimer.com
Summary
goose configurewill now show the correct config files updated after applying changes. Updates the initial message to suggest that users can change the configs in~/.config/gooseNote: This is my first time coding in Rust! 😝
Type of Change
Testing
Manual testing
Related Issues
Relates to #5117 , closes #5196