Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #860 +/- ##
==========================================
- Coverage 86.67% 85.18% -1.49%
==========================================
Files 48 50 +2
Lines 5244 5300 +56
Branches 5244 5300 +56
==========================================
- Hits 4545 4515 -30
- Misses 481 565 +84
- Partials 218 220 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR moves the settings file location from the current working directory to a platform-appropriate config directory and adds CLI commands to manage the settings file. The change improves user experience by making settings persistent regardless of the working directory.
- Relocates
settings.tomlfrom CWD to OS-specific config directory (e.g.,~/.config/muse2/settings.toml) - Adds new
settingsCLI subcommands for editing, deleting, showing path, and dumping default settings - Updates documentation to reflect the new settings management workflow
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Adds get_muse2_config_dir() function to determine platform-specific config directory |
| src/settings.rs | Updates settings loading to use config directory instead of CWD, adds path helper function |
| src/cli/settings.rs | Implements new CLI subcommands for settings file management |
| src/cli.rs | Replaces DumpDefaultSettings command with new Settings subcommand group |
| docs/user_guide.md | Updates documentation to show new settings management workflow |
| Cargo.toml | Adds dependencies for directory resolution (dirs) and file editing (edit) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I've found a bug (on |
Also add a `settings path` command so the user can find where this is.
|
Everything should be working now, so this is good to review. |
dalonsoa
left a comment
There was a problem hiding this comment.
I cannot test it today - wrong laptop :P - but will do so first thing tomorrow.
Otherwise, it looks OK. I just have a minor comment.
src/cli/settings.rs
Outdated
| /// Get the path to where the settings file is read from | ||
| Path, | ||
| /// Write the contents of a placeholder `settings.toml` to the console | ||
| DumpDefault, |
There was a problem hiding this comment.
I would call this View to see the current settings in the terminal, which would be the default ones if there are no custom settings configured. If you want, you can add a ViewDefault, which always shows the default values, regardless what the current settings are.
There was a problem hiding this comment.
Good idea! I've named them show and show-default instead.
|
Works well. In my case, in MacOS, the |
Interesting. Well, at least it picked something usable... If you set the |
Co-authored-by: Diego Alonso Álvarez <6095790+dalonsoa@users.noreply.github.com>
tsmbland
left a comment
There was a problem hiding this comment.
This is cool! edit opens vscode for me. Apart from what Diego suggested which I agree with, two points:
settings pathgives a path even if there's no settings file - is this intentional?- All the parameters are commented out when I first open the settings file, but I think this is a bit unclear and unnecessary
Yep. I thought it was useful to tell the user where the settings file should live, even if it's not there. But I suppose we could also print an error saying the file isn't present (preferably also including the file path).
This is a pretty common thing to do with placeholder config files. If the user wants to change something explicitly they can just uncomment whatever line they want and at least then we know that any option that's been set was done on purpose. If they were all uncommented by default, that means that if we change the default value in a later version of MUSE2 (or even make the old default value invalid), then the user will get potentially strange behaviour if they have ever run |
I'd probably go with the latter but totally your call
Good point. I didn't think about that. In that case, I would just add a note in the file explaining that the comments show the defaults at the time of file creation (not necessarily the current defaults), and to uncomment any parameters that they want to change |
I'm happy with that. I'll fix up.
Sure. The more documentation the better 😄 |
Description
This has been bugging me for a while, so I've had a go at it, even though it's low priority.
Currently the program settings are loaded from a
settings.tomlfile in the CWD, if one exists, but this isn't ideal -- users should be able to change directory without the program settings changing!There are standard places to store program config files (differing between OSes), so we should use this as a place to store the settings file instead. However, in doing this, it may make it harder for users to figure out where they should be creating their settings file etc., so I think it makes sense to add some helper commands to make this easier.
I've added a
settings editcommand, which allows users to edit the program settings. If the settings file doesn't exist, a placeholder file with the options commented out will be created. I'm using this crate to open the file with an appropriate text editor and I think whatever default heuristics it uses should be fine. I'd be interested to know what happens if you try it on your own systems though!Besides that, I've also added some other commands that should be self explanatory.
Closes #255.
Type of change
Key checklist
$ cargo test$ cargo docFurther checks