Repository navigation
fix(config): preserve set values in config set - #10647
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughConfig and settings set inputs now normalize comma-separated values by trimming, removing empty entries, and deduplicating arrays. E2e coverage was added for ChangesSet value normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/cli/test_config_set (1)
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the persisted TOML too.
This only checks the read path. Because
settings.disable_toolsis normalized upstream,mise config getcan still return["node", "rust"]even ifconfig setwrote duplicates or spacing incorrectly. Add acat mise.tomlassertion here to pin the storage contract described in the PR.Suggested assertion
mise config set settings.disable_tools node,rust,node assert "mise config get settings.disable_tools" '["node", "rust"]' +assert "cat mise.toml" $'[tools.node]\nversion = "latest"\n\n[settings]\ndisable_tools = ["node", "rust"]'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/cli/test_config_set` around lines 16 - 17, The test around mise config set for settings.disable_tools only verifies the read path via mise config get, so it can miss duplicate or spacing bugs in persisted output. Update this e2e case to also assert the contents of mise.toml after the mise config set call, using the same settings.disable_tools scenario, so the storage contract is pinned in addition to the normalized read result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/config/set.rs`:
- Around line 136-139: The array-building logic in set::set_config currently
trims each comma-separated segment but still pushes empty strings into the
toml_edit::Array, so update that loop to skip any item that becomes empty after
trimming. Use the existing value.split(',') handling in set::set_config and its
toml_edit::Array population to filter out blank entries before calling push.
---
Nitpick comments:
In `@e2e/cli/test_config_set`:
- Around line 16-17: The test around mise config set for settings.disable_tools
only verifies the read path via mise config get, so it can miss duplicate or
spacing bugs in persisted output. Update this e2e case to also assert the
contents of mise.toml after the mise config set call, using the same
settings.disable_tools scenario, so the storage contract is pinned in addition
to the normalized read result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a8b31c49-4338-4c2b-a518-1a3f2ebdd929
📒 Files selected for processing (2)
e2e/cli/test_config_setsrc/cli/config/set.rs
Greptile SummaryThis PR fixes two bugs in
Confidence Score: 5/5Safe to merge — changes are narrowly scoped to Set-type parsing in two CLI commands, with no behavioral changes to other types. Both the type-inference fix and the normalization logic are straightforward and well-tested. The nested key lookup correctly delegates to the already-generated SETTINGS_META dotted keys. The Set branch and parse_set_by_comma now handle empty input, "[]", whitespace-wrapped "[]", sparse commas, and duplicates consistently with each other. No files require special attention. Important Files Changed
Reviews (7): Last reviewed commit: "fix(config): preserve set values in conf..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
befeea6 to
c18effa
Compare
f7fe7cd to
c53699e
Compare
|
nightly job seems broken on main as well (ICE while checking rattler_package_streaming, specifically around DynCompatible(DownloadReporter)/reqwest::tokio::get_reader.) |
f0dbae1 to
03a9309
Compare
`mise config set` inferred `SetString` settings like `settings.disable_tools`, but the set serialization path discarded the provided value and always wrote an empty array. Nested settings such as `settings.task.skip` also missed settings metadata because lookup only used the final key segment. Parse comma-separated set values before deduping, trim whitespace, ignore empty entries, and treat `[]` as an explicit empty set. Look up settings metadata using the full key after the `settings.` prefix so nested settings infer the right type. Apply the same empty-entry and whitespace handling to `mise settings set`, and update the config set help/docs example to show the inferred settings-type path instead of forcing `--type list`. Tests: - mise run test:e2e e2e/cli/test_config_set e2e/cli/test_settings_set
03a9309 to
721d6c9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Summary
mise config setfor set-typed settings so comma-separated values are preserved and deduplicated instead of writing an empty arraysettings.*metadata using the full key aftersettings., including keys likesettings.task.skipmise settings setSetString parsing consistent by trimming entries, dropping empty entries, deduplicating, and treating[]as an empty setdocs/cli/config/set.mdandmise.usage.kdl) to show the inferred settings-type path instead of forcing--type listOnly SetString parsing is changed; ListString/ListPath parsing is intentionally unchanged.
Testing
mise run test:e2e e2e/cli/test_config_set e2e/cli/test_settings_setcargo fmt --all -- --checkgit diff --checkmise run render:usagemise run lint./target/debug/mise config set --helpThis PR description was updated by an AI coding assistant.
Summary by CodeRabbit
Bug Fixes
settings.disable_toolsandsettings.task.skipnow normalize more reliably when set through the CLI.Documentation