Match commands with path prefixes in @for_app decorations #1133
Conversation
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
added 2 commits
July 7, 2021 14:16
Commands entered with a path do not match is_app. I encountered this when working with a test for the rm_dir rule. This rule did not use the @for_app decorator, but when I migrated it, the test for "./bin/hdfs.." failed because 'hdfs' was recognized as a command, while "./bin/hdfs" was not. This commit addresses the false negative by resolving path names in the command, via os.path.basename.
I presume that the `./` in `./gradlew` was used here because thefuck would not find an app match on just `gradlew`, and thus no fucks would be given on the most common and idiomatic way of invoking gradlew. After 8faf9b1, thefuck does not distinguish between commands with paths and those without. Therefore, the tests for this rule are now broken because thefuck strips paths from the _user_'s command, but not from the for_app decoration. This commit addresses that problem by changing the for_app decoration to this rule.
scorphus
approved these changes
Jul 7, 2021
Collaborator
scorphus
left a comment
scorphus
left a comment
There was a problem hiding this comment.
Thanks, @rpdelaney! Apologies for the delay.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
I was working on an extension to the
rm_dirrule (to tryrmdirbeforerm -r, since it's safer) and discovered that I could not switch to using the@for_appdecorator because then the rule loses its match from/bin/hdfs dfs -rmtohdfs.If
git diffmatches ongit, then/usr/bin/git diffshould match also. Not matching on explicit paths:+Gives rule authors the ability to match only when an explicit path to the command is given-Prevents rule authors from matching on a command regardless of what path was given (if any)Since the default behavior for
fuckis to require user interaction before confirming a command, casting a wider net is unlikely to be harmful.Also, the only rule that uses
@for_appand has a path in the match string that I could find, has the path there because the most common invocation of that tool uses a path and the rule doesn't work otherwise, because of this limitation. In other words, it seems that the only rule that uses a path in the for_app string does so as a work-around to this too-strict behavior. I can only speculate that other rules, including the rm_dir rule, may have avoided the decorator for this reason.Rule authors should not have to enumerate all the different paths a command could be called from, in my view. Hopefully this has not been litigated previously.
Before accepting, consider my commit message to d30a789. It may be better to strip paths in both places, but I'm not sure. Also if you need more tests, let me know where and I'll be happy to add them.