Unify boolean CLI flags and settings.toml options#1377
Open
alexdewar wants to merge 3 commits into
Open
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:
|
Contributor
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.
Comment on lines
+135
to
137
| // 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() { |
Comment on lines
+44
to
47
| /// Settings that can be overridden on the command line | ||
| #[command(flatten)] | ||
| pub overrides: SettingsOverrides, | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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