Unify boolean CLI flags and settings.toml options#1377
Conversation
This reverts commit b12a8f3.
…alse` This is for consistency, to make it easier to unify CLI and settings file interfaces.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1377 +/- ##
==========================================
+ Coverage 89.68% 89.74% +0.06%
==========================================
Files 58 58
Lines 8406 8399 -7
Branches 8406 8399 -7
==========================================
- Hits 7539 7538 -1
+ Misses 550 548 -2
+ Partials 317 313 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors how boolean options are defined and resolved by introducing a single source of truth for defaults plus a unified “settings file + CLI override” merge path, so CLI-provided values take precedence over settings.toml.
Changes:
- Introduces a
settings!macro that declares each setting once and generatesSettings, defaults,SettingsOverrides, andSettings::apply_overrides. - Updates the
runcommand (and anything reusingRunOpts, e.g.example run) to useSettingsOverridesvia#[command(flatten)]instead of manual precedence logic. - Updates CLI/regression tests to use the new boolean flag style (e.g.
--copy-input-files=false).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/regenerate_test_data.sh | Updates example-run invocation to the new copy-input-files boolean flag style. |
| tests/cli.rs | Updates CLI integration test to use --copy-input-files=false (and renames the test). |
| src/settings.rs | Adds macro-generated settings + CLI override struct and a precedence test for overrides. |
| src/cli.rs | Switches run to SettingsOverrides flatten + merge; adjusts save-graphs overwrite handling to allow explicit opt-out. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Command-line arguments take precedence over the settings file | ||
| settings.apply_overrides(&opts.overrides); | ||
|
|
|
|
||
| #[test] | ||
| fn check_no_copy_input_files_flag() { | ||
| fn check_copy_input_files_flag() { |
| /// Settings that can be overridden on the command line | ||
| #[command(flatten)] | ||
| pub overrides: SettingsOverrides, | ||
| } |
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Fun experiment, although I'm not too sure about this to be honest. Struggled to understand some of the reasoning - not that it's necessarily bad, I think just needs more effort to make it understandable for a human reviewer.
In practice, we've only got a few settings and rarely add new ones, so I'm not too concerned about the manual work of adding new settings (copilot may flag if we forget something anyway). I agree that we should use consistent syntax between settings.toml and the CLI, and I like the ability to use =false.
We probably want some manual control over command line flags anyway, as not all flags will apply to all commands. Maybe this retains that control, but not really sure what the added benefits are. I can also imagine that if we added more complex arguments to settings.toml like lists, integers or whatever, then the macro could end up very big and complicated in an attempt to generalise all this (not that we'd necessarily want to go down that route).
Overall, I think there are some small improvements here that we should certainly go ahead with, and could probably be persuaded on the whole approach, but need a bit more convincing.
| /// Whether to overwrite the output directory if it already exists | ||
| #[arg(long)] | ||
| pub overwrite: bool, | ||
| #[arg(long, value_name = "BOOL", num_args = 0..=1, require_equals = true, default_missing_value = "true")] |
There was a problem hiding this comment.
Why do we still have overwrite here and not in RunOpts?
| } | ||
|
|
||
| /// Handle the `validate` command. | ||
| pub fn handle_validate_command(model_path: &Path) -> Result<()> { |
There was a problem hiding this comment.
Why not doing the same thing for this command?
| ); | ||
| }; | ||
|
|
||
| // File-only setting: skip it. |
| "--output-dir", | ||
| &output_dir.to_string_lossy(), | ||
| "--no-copy-input-files", | ||
| "--copy-input-files=false", |
There was a problem hiding this comment.
I think this is an improvement
| settings!(@overrides { $($fields)* } { $($applies)* } $($rest)*); | ||
| }; | ||
|
|
||
| // No settings left: emit the overrides struct and the merge method. |
There was a problem hiding this comment.
What about settings that are neither bool or file paths?
Description
There are three boolean options for MUSE2 which can be set either via command-line arguments or made persistent by editing the
settings.tomlfile. The command-line argument, if present, always takes precedence. The current code involves setting default values in multiple places and the override logic is implemented manually.I wanted to see if Copilot could come up with a way to unify the two and make it tidier and hopefully less error prone and it wrote a big macro in
settings.rswhich does the trick. I won't pretend to understand the intricacies of Rust's macro syntax, so I'm treating it as a bit of a black box.I think on balance it is an improvement, but I thought I'd open a PR for it to see what everyone else thinks. I can also see the argument for keeping the code understandable!
It seems the macro can be extended to accept string options too, in which case we could change the current approach of making the program log level settable with an environment variable to using a command-line argument for that too, but that change seemed out of scope for this PR.
A consequence of this change is that you can explicitly set command-line options to true/false (
--debug-model=false), but defaulting to true, as before.Closes #1284.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks