Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use _command_exists everywhere #1938

Merged
merged 36 commits into from
Sep 20, 2021
Merged

Use _command_exists everywhere #1938

merged 36 commits into from
Sep 20, 2021

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Sep 10, 2021 •
edited
Loading

Description

Use _command_exists instead of command -v xx &>/dev/null (and similar) throughout the code base.

Motivation and Context

I grep'd for type and command -v and declare -F and fixed everything I could find. This closes #1632.

How Has This Been Tested?

All tests pass, but real testing hasn't been done as of this writing.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@gaelicWizard gaelicWizard force-pushed the _command_exists branch 9 times, most recently from 108da26 to 955fa2f Compare September 12, 2021 06:52
@gaelicWizard gaelicWizard force-pushed the _command_exists branch 3 times, most recently from 50988ae to cd9654e Compare September 19, 2021 04:00
@gaelicWizard gaelicWizard changed the title DRAFT: use _command_exists throughout Use _command_exists throughout Sep 19, 2021
@gaelicWizard gaelicWizard marked this pull request as ready for review September 19, 2021 07:49
@gaelicWizard
Copy link
Contributor Author

Rebased on current master and ready for review. 😃

@gaelicWizard gaelicWizard changed the title Use _command_exists throughout Use _command_exists everywhere Sep 19, 2021
@gaelicWizard
Copy link
Contributor Author

@cornfeedhobo you said you might have time this weekend, so here's a PR 😆

@cornfeedhobo
Copy link
Member

@gaelicWizard I like the spirit of this PR, but I'm not comfortable merging it since there are so many other changes that are unrelated. I'm not going to block the PR in case @NoahGorny or @davidpfarrell want to move this forward, but I would prefer to limit these type of horizontal clean-ups to a singular change, because it spans so many files. If you'd like to open up another PR that only changes which/command to _command_exists, I'll happily test and review that, and then you could submit these other changes in individual PRs.

As always, thanks so much and let me know if I can help somehow.

@davidpfarrell
Copy link
Contributor

davidpfarrell commented Sep 19, 2021 •
edited
Loading

Hi All !

So @cornfeedhobo is right there really appears to be a lot of unrelated changes here.
I'm guessing this is a stacked PR built on another PR?

(assuming yes)

I think it might be best to tag ALL stacked PRs as DRAFT and untag PRs one at a time as they become clean and ready for [final] review. Of course casual reviews can happen on DRAT PRs too as reviewer's time permits ...

[edit] Clarified final vs casual reviews ...

@gaelicWizard gaelicWizard changed the title Use _command_exists everywhere DRAFT: Use _command_exists everywhere Sep 19, 2021
@gaelicWizard gaelicWizard marked this pull request as draft September 19, 2021 17:08
@gaelicWizard
Copy link
Contributor Author

Sounds fair! I'll split this up 👍

@gaelicWizard gaelicWizard changed the title DRAFT: Use _command_exists everywhere Use _command_exists everywhere Sep 19, 2021
@gaelicWizard gaelicWizard marked this pull request as ready for review September 19, 2021 17:28
@gaelicWizard
Copy link
Contributor Author

@cornfeedhobo, @davidpfarrell, I've split out all the files that had more than just _command_exists and I'll submit those separately. This one should be good to go now! 👍

@gaelicWizard gaelicWizard mentioned this pull request Sep 19, 2021
Merged
6 tasks
@gaelicWizard
plugins/virtualenv: use _command_exists
ccd8b52
@gaelicWizard
plugins/ruby: use _command_exists
b14bb47
@gaelicWizard
plugins/rbenv: use _command_exists
cff6f34
@gaelicWizard
plugins/pyenv: use _command_exists
a311453
@gaelicWizard
plugins/plenv: use _command_exists
f0179c7
@gaelicWizard
plugins/nodenv: use _command_exists
b038ea5
@gaelicWizard
plugins/direnv: use _command_exists
c984243
@gaelicWizard
completion/travis: use _command_exists
ef0c643
@gaelicWizard
completion/pew: use _command_exists
bb555ae
@gaelicWizard
completion/consul: use _command_exists
dca96e0
@gaelicWizard
completion/awscli: use _command_exists
699d893
@gaelicWizard
aliases/general: use _command_exists
6618457
@gaelicWizard
aliases/curl: use _command_exists
88d66bb
@gaelicWizard
aliases/apt: use _command_exists
0ad1af8
@gaelicWizard
completion/gradle: use _command_exists
679d8b1
@gaelicWizard
completion/docker-compose: use _command_exists
699720f
@gaelicWizard
completion/hub: use _command_exists
8e9438d
@gaelicWizard
lib/theme: use _command_exists
eabdf41
@gaelicWizard
theme/dulcie: use _command_exists
a2e32f3
@gaelicWizard
lib/helpers: simplify _command_exists() and _binary_exists()
8a03f45 
Remove subshell and just use a regular `if`
@gaelicWizard
completions/sqlmap: use _command_exists
fb6e05d 
Addresses Bash-it#1632
@gaelicWizard gaelicWizard mentioned this pull request Sep 20, 2021
Merged
6 tasks
cornfeedhobo
cornfeedhobo approved these changes Sep 20, 2021
View reviewed changes
Copy link
Member

@cornfeedhobo cornfeedhobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! Thank you for the hard work!!

cc @NoahGorny

NoahGorny
NoahGorny approved these changes Sep 20, 2021
View reviewed changes
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done @gaelicWizard, and of course thanks for the review @cornfeedhobo ❤️

@NoahGorny NoahGorny merged commit 8c69771 into Bash-it:master Sep 20, 2021
@gaelicWizard gaelicWizard deleted the _command_exists branch September 20, 2021 18:49
@davidpfarrell davidpfarrell mentioned this pull request Sep 21, 2021
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NoahGorny NoahGorny NoahGorny approved these changes

@cornfeedhobo cornfeedhobo cornfeedhobo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure that _command_exists is used consistently in all components
4 participants
@gaelicWizard @cornfeedhobo @davidpfarrell @NoahGorny