-
Notifications
You must be signed in to change notification settings - Fork 140
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
feat(fish): use abbr instead of alias #348
Conversation
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.
Overall this is a MASSIVE improvement. I have not tested it yet (but will do so after some chatting).
I especially like the extreme reduction in duplication, having "one source of truth" for commands and completions etc.
I'm worried mainly about affecting the user's $PATH in a way that is invasive. I would be surprised if I sourced a plugin and my PATH was affected.
conf.d/forgit.plugin.fish
Outdated
"$FORGIT" ignore_get $argv | ||
end | ||
# Add `git-forgit` command to PATH | ||
fish_add_path -ga "$FORGIT_INSTALL_DIR/bin" |
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.
Is there any way to do this without affecting the user's $PATH? That seems like it could be a little invasive?
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 can replace git-forgit
in the abbreviations with $FORGIT
. Completion shouldn't be affected, but the expanded command line might look a bit ugly (fish expands the full path of $FORGIT_INSTALL_DIR/bin/git-forgit
.
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.
Meaning when you expand the abbreviation?
It would expand:
ga
becomes
$FORGIT_INSTALL_DIR/bin/git-forgit add
For the user?
Instead of
git forgit add
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 added git-forgit
alias to the full-path command, so hopefully it'll solve the "lengthy command line" problem.
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.
Great! Let me try this out for a bit and get a feel for it before merging. Overall code structure wise it's a big improvement, just going to perform some testing to make sure I understand it and there aren't any gotchas.
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.
Still testing this! It's definitely a change as a user, but easy to adapt to. It also de-duplicates the code so much, which is a huge positive.
bc52129
to
1bd66bb
Compare
conf.d/forgit.plugin.fish
Outdated
@@ -2,10 +2,8 @@ | |||
|
|||
set INSTALL_DIR (dirname (dirname (status -f))) | |||
set -x FORGIT_INSTALL_DIR "$INSTALL_DIR/conf.d" | |||
set -x FORGIT "$FORGIT_INSTALL_DIR/bin/git-forgit" |
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.
Unfortunately this line cannot change, this is auto-replaced in our homebrew script, so it should stay as it is! I would revert this entire section (5-8)
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.
@cjappl The line has just been moved (to line 8), why can't it be auto-replaced there now?
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.
Perhaps it could but it would need testing with the homebrew formula to make sure it still works. If this change can be done with keeping the original structure in place it's one less thing to check.
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 removed the 2nd commit modifying the variable.
1bd66bb
to
26c8ebb
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 think let's run with this for now. I think the code maintenance is substantially improved and the minor backwards compatability stuff (not being able to use abbreviations in scripts for fish) is easily fixable by clients.
I'm going to merge and we can always revert if there is a riot.
Thanks again for the fix on this, I had no idea the power of abbr !
Check list
Description
string collect
returns 1 on empty argument, so it's used here to avoid repeatingif else end
blocks. Another benefit is that the output ofstring collect
is ensured to be a single string. Fromstring --help
:This PR is marked as breaking change, since abbr behaves differently from alias. It can only be used in the interactive command line (so putting
exec glo
into your scripts won't work).Type of change
Test environment