Add wrong_hyphen_before_subcommand rule
#977
Conversation
|
I didn't abandon this PR to fail, currently working on mock-ups for the man-page retrieving functions. |
| if subcmd in synopsis or find_subcommand(manpage, cmd, subcmd): | ||
| # We are dealing with an accidentally added hyphen | ||
| return command.script.replace('-', ' ', 1) | ||
|
|
||
| else: | ||
| # We are dealing with a missing space | ||
| return command.script.replace('-', ' -', 1) |
There was a problem hiding this comment.
Perhaps a simpler version of the entire rule would be better.
IMO, TheFuck should delegate that choice to the user.
Following the path the current rule is going, the failed command apt-install foo would get two suggestions, apt -install foo and apt install foo, among others.
Thing is that the former – apt -install foo – already gets suggested by the existing missing_space_before_subcommand rule.
Looking at it that way, all the wrong_hyphen_before_subcommand – or even an augmented missing_space_before_subcommand – rule would have to do is replace the hyphen with a space.
What you think?
There was a problem hiding this comment.
Following your suggestions, I tried to simplify the rule down. I'm afraid I over-simplified it (since it may now suggest git hello as a replacement to git-hello even though hello is not a legitimate subcommand for git).
I'd be happy to hear your opinion about that. Should rules do above and beyond to verify their command suggestions? Or should the current way of command checking be the norm?
Anyway, another issue I have is that the rule now should not try to handle cases such as ls-la (-> ls -la), but as it doesn't check the manpages anymore there is no way for the rule to determine if la is a subcommand or flags. Thus it suggests ls la. Now the issue is that the priority of the rule (as I see it) should be lower than no_command's priority, and the missing_space_before_subcommand is higher than no_command's. Thus the wrong_hyphen_before_subcommand shadows the missing_space_before_subcommand rule.
Any suggestions?
There was a problem hiding this comment.
I think it's better if the rule does not try to be super smart. In the same sense as missing_space_before_subcommand. Let it suggest any fix that has some probability to work. The user decides to accept. That's my own opinion.
Regarding the priority, I think it should be set to 3900. It's more close to missing_space_before_subcommand than to no_command.
@nvbn wanna chime in, pal?
README.md
Outdated
| @@ -299,6 +299,7 @@ following rules are enabled by default: | |||
| * `vagrant_up` – starts up the vagrant instance; | |||
| * `whois` – fixes `whois` command; | |||
| * `workon_doesnt_exists` – fixes `virtualenvwrapper` env name os suggests to create new. | |||
| * `wrong_hyphen_before_subcommand` – adds or omits dashes improperly placed (`ls-la` -> `ls -la`, `git-log` -> `git log`, etc.) | |||
There was a problem hiding this comment.
The description lacks an update.
| if '-' not in command.script_parts[0] or command.script_parts[0] in get_all_executables(): | ||
| return False | ||
|
|
||
| cmd, subcmd = command.script_parts[0].split('-')[:2] |
There was a problem hiding this comment.
Now that subcmd isn't necessary anymore, this line could be changed to:
| cmd, subcmd = command.script_parts[0].split('-')[:2] | |
| cmd = command.script_parts[0].split('-', 1)[0] |
|
|
||
| @sudo_support | ||
| def get_new_command(command): | ||
| return command.script.replace("-", " ", 1) |
There was a problem hiding this comment.
This is dangerous, there might be other hyphens. You really should only replace the hyphen in script_parts[0].
There was a problem hiding this comment.
I would agree if the match function wouldn't require a hyphen in command.script_parts[0].
| return False | ||
|
|
||
| cmd, subcmd = command.script_parts[0].split('-')[:2] | ||
| if cmd not in get_all_executables(): |
There was a problem hiding this comment.
This could simply be :
return cmd in get_all_executables()There was a problem hiding this comment.
@yotam180, can you please address this remark?
| if cmd not in get_all_executables(): | |
| return cmd in get_all_executables() |
This along with the one a few boxes above:
- cmd, subcmd = command.script_parts[0].split('-')[:2]
+ cmd = command.script_parts[0].split('-', 1)[0]
scorphus
force-pushed
the
dev/wrong-hyphen
branch
2 times, most recently
from
d4bae68 to
2a9d92f
Compare
scorphus
force-pushed
the
dev/wrong-hyphen
branch
from
2a9d92f to
a07955a
Compare
This rule corrects two common mistakes:
ls-lainstead ofls -la)apt-installand notapt install)Regarding issue #975