-
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
lib/helpers: fix _command_exists()
#2018
Conversation
@NoahGorny, this one is tiny tiny if you could review & approve ♥ |
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.
Greetings! Got some feedback on this one, but generally happy to see it get some attention ...
3fd97b4
to
5d72e62
Compare
This is ready for merge unless someone wants to OK removing the default message 👍 |
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'd rather remove the default message
Wondering what that would look like? i.e would we lose $2
altogether as well?
As these checks are often the core reason that components bail early and skip activation, I feel like its probably important for some form of logging to show up in the bash-it doctor logs.
I also feel like, in general, we are not passing $2
along when we invoke these (even in newly added code), nor are we adding our own logging statements.
So both removing the default message OR removing logging altogether feel like missed opportunities to give the user useful bash-it doctor messages.
It seems like only a few plugins/components actually do this, and it feels like logging about why bailing early should be in the plugin. If I definitely agree there should be some message when a plugin/completion is skipped, but it should be logged from the component script not the |
3efdb38
to
2b4eee3
Compare
@davidpfarrell, here's an example I just made up for
Since we're bailing early, we can't just Compare to
EDIT: ok, well, it seems I've been nerd-sniped in to this being my next project. I do need at least one specific bit of assistance: should this be |
2b4eee3
to
e6e570a
Compare
Ok, I've got a branch I'm working on for early-bail from plugins that require other commands but it's way to big for this PR simply due to the number of files. I think this branch should be good to merge, and then I'll open a new PR when I've got all the plugins ready and we can revisit how the logging should flow then. 👍 |
I would say its a warning and not an error- error is something truly not working properly in Bash-it IMO |
e6e570a
to
170d70d
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.
Approved !
- Don't write to disk, just pipe. - Don't loop, just do all functions. Performance of old implementation on my system: ``` real 0m9.996s user 0m5.318s sys 0m9.126s ``` Performance of new implementation on my system: ``` real 0m0.052s user 0m0.069s sys 0m0.025s ```
The weird subshell is weird AF. Just do a normal `if`. Ditto `_binary_exists()`, `_completion_exists()`, and `_is_function()`!
170d70d
to
9b51dc0
Compare
This is a tiny PR since I'm having trouble rebasing my
helpers
PR (#1934). This change was in the middle and just much easier to pull it aside 😃Description
The weird subshell is weird AF. Just do a normal
if
. Ditto_binary_exists()
,_completion_exists()
, and_is_function()
! Alsö, makeall_groups()
literally 100x faster.Motivation and Context
Simplicity, consistency, and #1696! And
git rebase
is hard sometimes.How Has This Been Tested?
Locale and all tests pass.
Types of changes
Checklist:
clean_files.txt
and formatted it usinglint_clean_files.sh
.