feat(config): accept --file on mise unuse and --path on mise unset - #11616
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 as they are similar to previous changes (6)
📝 WalkthroughWalkthrough
ChangesCLI option aliases
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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_target_aliases (1)
26-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for directory targets.
These tests exercise the new aliases with
alias.toml, which is a file. The PR objective also promises file and directory targets. Add at least one directory-target case forunuse --fileandunset --path, or point to existing end-to-end coverage for those paths.🤖 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_target_aliases` around lines 26 - 42, Extend the config target alias coverage to include directory targets, not only the file target alias.toml. Add at least one end-to-end case exercising unuse --file and one exercising unset --path against a directory target, verifying the corresponding entry is removed while preserving the existing file-target and canonical-name checks.
🤖 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 `@man/man1/mise.1`:
- Around line 4288-4291: Update the source help text for the mise unset
--file/--path option to use <PATH> instead of <FILE> and document how file and
directory targets are resolved. Preserve the existing default and global-config
path details, then regenerate the generated man page so man/man1/mise.1 reflects
the source help.
---
Nitpick comments:
In `@e2e/cli/test_config_target_aliases`:
- Around line 26-42: Extend the config target alias coverage to include
directory targets, not only the file target alias.toml. Add at least one
end-to-end case exercising unuse --file and one exercising unset --path against
a directory target, verifying the corresponding entry is removed while
preserving the existing file-target and canonical-name checks.
🪄 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: fc734979-458f-44f5-8153-7d3d15178765
📒 Files selected for processing (5)
e2e/cli/test_config_target_aliasesman/man1/mise.1mise.usage.kdlsrc/cli/unset.rssrc/cli/unuse.rs
| \fB\-f, \-\-file, \-\-path\fR \fI<FILE>\fR | ||
| Specify a file to use instead of `mise.toml` | ||
|
|
||
| Defaults to [`MISE_DEFAULT_CONFIG_FILENAME`](https://mise.jdx.dev/configuration.html#mise_default_config_filename) environment variable, or `mise.toml`. Use [`MISE_GLOBAL_CONFIG_FILE`](https://mise.jdx.dev/configuration.html#mise_global_config_file) to choose a different global config path. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document directory targets for mise unset.
The option is rendered as <FILE> and says “Specify a file”, but mise unset accepts files and directories. Update the source help text to use <PATH> and describe directory resolution, then regenerate man/man1/mise.1.
Suggested wording
-\fB\-f, \-\-file, \-\-path\fR \fI<FILE>\fR
-Specify a file to use instead of `mise.toml`
+\fB\-f, \-\-file, \-\-path\fR \fI<PATH>\fR
+Specify a config file or directory to use instead of `mise.toml`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| \fB\-f, \-\-file, \-\-path\fR \fI<FILE>\fR | |
| Specify a file to use instead of `mise.toml` | |
| Defaults to [`MISE_DEFAULT_CONFIG_FILENAME`](https://mise.jdx.dev/configuration.html#mise_default_config_filename) environment variable, or `mise.toml`. Use [`MISE_GLOBAL_CONFIG_FILE`](https://mise.jdx.dev/configuration.html#mise_global_config_file) to choose a different global config path. | |
| \fB\-f, \-\-file, \-\-path\fR \fI<PATH>\fR | |
| Specify a config file or directory to use instead of `mise.toml` | |
| Defaults to [`MISE_DEFAULT_CONFIG_FILENAME`](https://mise.jdx.dev/configuration.html#mise_default_config_filename) environment variable, or `mise.toml`. Use [`MISE_GLOBAL_CONFIG_FILE`](https://mise.jdx.dev/configuration.html#mise_global_config_file) to choose a different global config path. |
🤖 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 `@man/man1/mise.1` around lines 4288 - 4291, Update the source help text for
the mise unset --file/--path option to use <PATH> instead of <FILE> and document
how file and directory targets are resolved. Preserve the existing default and
global-config path details, then regenerate the generated man page so
man/man1/mise.1 reflects the source help.
Greptile SummaryThis PR makes the inverse config-target commands accept the same synonymous option names as their counterparts.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "feat(config): accept --file on `mise unu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
f1da22b to
8411bc5
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Both addressed, thanks — the first one was a real gap.
Fixed at the source ( I did not rename the placeholder from Directory coverage in the e2e. Added: the test now exercises a directory target through all four commands under the aliased names, which is also what makes the new help text checkable rather than just asserted — mise use --file subdir dummy@1
mise set --path subdir SUBDIR_VAR=via-dir
mise unuse --no-prune --file subdir dummy@1
mise unset --path subdir SUBDIR_VAR |
8411bc5 to
9a4769b
Compare
9a4769b to
4e43e26
Compare
2 participants
Follow-up to #11577, which left the pair half-finished:
mise use --filemise unuse --filemise set --pathmise unset --pathSo the command that adds a tool takes both spellings and the command that removes it does not. This adds
--fileas a visible alias formise unuse --path, and--pathformise unset --file— the same one-line change #11577 made, applied to the two inverse commands.The alias is accurate, not just a spelling: both commands accept a directory as well as a file.
unsetresolves throughresolve_target_config_path, which has apath.is_dir()branch, andunusecallsconfig::config_file_in_dirwhen the argument is a directory.Background
#4881 asks for
--file/--pathas synonyms on the config-target commands. #11116 implemented it across eleven commands and was closed the next day. #11577 narrowed it to the two the discussion actually names, and that merged — so I am walking the rest in small steps rather than re-proposing the wide change.Remaining after this one:
config get,config set,dotfiles add, and the fourbootstrap packagescommands (use,import,brew tap,brew untap).Generated files
mise.usage.kdlandman/man1/mise.1carry the four expected lines. They were written by hand from the shape #11577's merged output already has in-tree (mise setat man:3295,mise useat man:4468), somise run rendershould be a no-op — thelintjob will say so either way.Tests
e2e/cli/test_config_target_aliases— the file #11577 added — gains the inverse half:unuse --fileandunset --pathremove whatuse --fileandset --pathwrote, and the canonical spellings still work afterwards.Summary by CodeRabbit
New Features
--pathas an alternative to--fileformise unset.--fileas an alternative to--pathformise unuse.mise.toml.Documentation
Tests