-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
aliases: Add new git-omz alias file #1831
base: master
Are you sure you want to change the base?
Conversation
Anyone else torn on whether we should use |
I went with |
3087913
to
90fe8dc
Compare
Hey @bittner, wanna take a look? |
How will we keep this aligned with the Oh My Zsh project implementation? In PR #1201 I tried to document a scripted approach. Would it make sense to do something similar here? |
This is indeed a problem, as the plugin will not work right out of the box.. The workflow might be |
How many approvals does a PR need? 🤪 |
haha, I still need to add some kind of script that updates this completion.. so its not ready yet. |
90fe8dc
to
e60c29a
Compare
I am also okay with vendoring it as it is right now- and update it once in a while. what do you think @cornfeedhobo @bittner ? |
May I suggest that the new alias file be named |
e60c29a
to
374f436
Compare
Nice idea- done! |
git-vendor-name: ohmyzsh git-vendor-dir: vendor/github.com/ohmyzsh/ohmyzsh git-vendor-repository: https://github.com/ohmyzsh/ohmyzsh.git git-vendor-ref: master
374f436
to
9eceb42
Compare
9eceb42
to
f7267d3
Compare
Howdy folks! |
@NoahGorny I like it. The only question I have remaining is if you are open to the idea of applying patches to vendored libs and what that should look like. I still need to submit a patch to deal with pre-exec's clobbering of |
@cornfeedhobo, I've already submitted a PR upstream for preexec's history shenanigans, based on an existing PR that they had been ignoring. They did accept an unrelated PR for unbound parameters, but no traction on this one yet. As for weird function names, we can handle that in our loader. I'll give it a go and post it here later tonight. I'd like to suggest that it's quite valuable to avoid, whenever possible, carrying any patches over vendor'd libs. I like Homebrew's stance on this: only carry patches that are already approved or merged in an upstream dev branch. But, that said, it might not be entirely right for Bash It. |
@gaelicWizard I think you saw my open issue with preexec. They aren't going to budge on this one. We either maintain a fork in our github organization, and patch that way, or figure out a scheme for patching on PR and documenting the diff. |
@cornfeedhobo, I did a whole PR which keeps their desired behavior in place, but behaves properly when we change it. My idea was to let them have their way, and then we just set |
The problem with patches is that we need to apply them somewhere, probably during installation. We need to think about how we do this and how other places are doing it. Simply having patches around isn't going to cut it sadly. I also think that committing directly into the vendored file like I did isn't great, but it is the simplest solution currently |
@@ -2,6 +2,9 @@ | |||
cite 'about-alias' | |||
about-alias 'common git abbreviations' | |||
|
|||
# We can use this variable to make sure that we don't accidentally clash with git-zsh aliases | |||
_bash_it_git_aliases_enabled=true |
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.
Can you use _bash-it-component-item-is-enabled()
?
_bash_it_git_aliases_enabled=true | |
if _bash-it-component-item-is-enabled aliases git-omz; then | |
_log_warning "The aliases from 'git' and from 'git-omz' conflict with each other; please only enable one." | |
return 1 | |
fi |
assert_line -n 0 -p ' aliases:' | ||
for alias in 'git' 'gitsvn' 'git-omz' | ||
do | ||
echo $alias |
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.
Doesn't the echo defeat the test?
@@ -0,0 +1,33 @@ | |||
# shellcheck shell=bash | |||
cite 'about-alias' |
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.
Shouldn't need this cite
; "about-alias" is cite
d in bash_it.sh
, so it never needs to be added to the alias files.
if [[ -n $_bash_it_git_aliases_enabled ]]; then | ||
_log_error "git-omz aliases are incompatible with regular git aliases" | ||
return | ||
fi |
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.
if [[ -n $_bash_it_git_aliases_enabled ]]; then | |
_log_error "git-omz aliases are incompatible with regular git aliases" | |
return | |
fi | |
if _bash-it-component-item-is-enabled aliases git; then | |
_log_warning "git-omz aliases are incompatible with regular git aliases" | |
return 1 | |
fi |
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 suggested use of _bash-it-component-item-is-enabled aliases git
. I think GitHub just lets you click accept if you like it and it commits it directly to the PR
Hello all, |
@NoahGorny Would you have a minute to resolve the conflicts of the PR and merge it, finally? That would be great! 💯 |
Hello again :) |
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 this is close to perfect.
@NoahGorny shall I merge it as is or do you want to give it the last round of improvements per the remarks posted?
Description
Adds git-zsh alias file, with aliases copied from ohmyzsh.
Currently there is no simple way of updating the aliases easily, and maybe we need to think about it.
Motivation and Context
Prompted by #1191, this is a re-iteration of #1201.
How Has This Been Tested?
Locally
Types of changes
Checklist:
clean_files.txt
and formatted it usinglint_clean_files.sh
.