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.

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

Plugins: bail early with descriptive messages #2062

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gaelicWizard
Copy link
Contributor

Description

Many plugins/completions/whatever check if their associated command is already installed before loading. Find, fix, and add in/to all the plugins to ensure consistency and improve bash-it doctor.

Motivation and Context

The _command_exist() (et al) functions were all weird due to having logging shoved in sideways, which was neither informative nor used anywhere. This pulls the logging from the utility functions and adds it to the relevant call sites, as well as adding to several other plugins that could use it.

In particular, some completions or some plugins may be useless but harmless if their associated command is not installed. I've opted to add logging, but not to force an early short-circuit. For example, I've made improvements to a lot of the completions code recently, but I don't have all the relevant tools installed. I load the completion and can test it out anyway, so I don't want to unnecessarily short-circuit it.

The result of all this should be a more reasonable output when $BASH_IT_LOG_LEVEL is set, and from bash-it doctor, as well as avoid polluting theme variables that check for installed utilities.

How Has This Been Tested?

Types of changes

  • Enhancement (non-breaking change which makes things better)

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 plugins-bail branch 6 times, most recently from 030c379 to ccd1bb7 Compare January 26, 2022 02:07
@gaelicWizard gaelicWizard changed the title Plugins: bail early with descriptive messages DRAFT: Plugins: bail early with descriptive messages Jan 26, 2022
@gaelicWizard gaelicWizard force-pushed the plugins-bail branch 4 times, most recently from 9995526 to b9e1535 Compare January 26, 2022 07:27
@gaelicWizard gaelicWizard changed the title DRAFT: Plugins: bail early with descriptive messages Plugins: bail early with descriptive messages Jan 26, 2022
@gaelicWizard gaelicWizard marked this pull request as ready for review January 26, 2022 07:31
@gaelicWizard gaelicWizard marked this pull request as draft January 26, 2022 07:31
@gaelicWizard gaelicWizard marked this pull request as ready for review January 26, 2022 07:36
@gaelicWizard gaelicWizard force-pushed the plugins-bail branch 2 times, most recently from 8609f5c to b535ffc Compare January 26, 2022 20:47
@gaelicWizard gaelicWizard force-pushed the plugins-bail branch 6 times, most recently from 571021a to 1646e5c Compare February 9, 2022 01:29
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.

Hi @gaelicWizard, how can we make sure that nothing is broken? this seems fine to me but this is changing a lot of plugins

@@ -1,42 +1,43 @@
# Load after the system completion to make sure that the fzf completions are working
# BASH_IT_LOAD_PRIORITY: 375
Copy link
Member

Choose a reason for hiding this comment

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

dont you want to keep it?

@seefood seefood added clean up seems abandoned rattle the cage, and close if nobody wants to keep it going waiting-for-response labels Nov 7, 2024
@seefood
Copy link
Contributor

seefood commented Nov 7, 2024

care to merge from root? at least 3 conflicts...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up seems abandoned rattle the cage, and close if nobody wants to keep it going waiting-for-response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants