-
-
Notifications
You must be signed in to change notification settings - Fork 660
feat(cli): add the ability to specify a custom keybinding/symbols file via the cli #2731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…e via the cli Introduce `-k`,`--key-bindings`, `-s`, and `--key-symbols` arguments to allow the user from specifying a custom config files without having to change the global ones this change is meant to address github issue: gitui-org#2714
c678978 to
5a14779
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the PR locally, I didn’t find any issues. There’s just a few suggestions for refactoring and a typo.
| Arg::new("key_symbols") | ||
| .help("Use a custom symbols file") | ||
| .short('s') | ||
| .long("key-symblos") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: this should by key-symbols.
| if !path.exists() { | ||
| return Err(anyhow!( | ||
| "The custom key bindings file dosen't exists" | ||
| )); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check can be removed as KeysList::init calls File::open which will error if the file does not exist.
| if !path.exists() { | ||
| return Err(anyhow!( | ||
| "The custom key symbols file dosen't exists" | ||
| )); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check can be removed as KeysList::init calls File::open which will error if the file does not exist.
| "The custom key bindings file dosen't exists" | ||
| )); | ||
| } | ||
| KeysList::init(path.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about something like KeysList::init(key_bindings_path.unwrap_or(Self::get_config_file()?))? (Same below for symbols.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t mean to approve just yet, this was by mistake.
Introduce
-k,--key-bindings,-s, and--key-symbolsarguments to allow the user from specifying a custom config files without having to change the global onesThis Pull Request fixes/closes #2714.
It changes the following:
I followed the checklist:
make checkwithout errors