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

Cleanup and load libraries explicitly #1823

Conversation

cornfeedhobo
Copy link
Member

@cornfeedhobo cornfeedhobo commented Jan 31, 2021

Description

  • remove all load loops and replace with explicit sourcing, and a commented reason and purpose
  • change ordering
    • for clarity and readability
    • to ensure that preexec is loaded last
  • minor clean-ups along the way, such as preferring double bracket tests with safer expansion and applying shfmt

Motivation and Context

I noticed that PROMPT_COMMAND didn't have uniform delimiters and I went digging for a reason.
I found that bash-preexec wants total control over this and DEBUG trap. So, if bash-it wants it to be a first-class option,
it will need to be loaded last.

This issue is loosely related to our discussion about vendoring and how to load libraries (@buhl @NoahGorny).
In that thread I argue that most of these loops are premature optimizations, given that our list of libraries isn't that long,
and in the section where we load theme components, we are perfectly happy to source them explicitly.
This is how I would do that.

I haven't removed the init.d directory because I assume it was made for good reason, but I don't see it, at least for preexec.
However, I want a discussion about it's efficacy before I return to using it or remove the file all together.

How Has This Been Tested?

manually and with bats

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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.

@cornfeedhobo
Copy link
Member Author

cornfeedhobo commented Jan 31, 2021

The biggest thing I'm currently worried about with this approach is that plugins are loaded before preexec is sourced, and I'm not sure how that impacts preexec_functions and precmd_functions array. I'll do some functional tests this evening...

@buhl
Copy link
Contributor

buhl commented Feb 1, 2021

You can test with the xterm.plugin.bash and cmd-returned-notify.plugin.bash plugins enabled. If your terminal is compatible it will set the title and mark the window with urgent hint when a long running command finishes.

@cornfeedhobo
Copy link
Member Author

Alright. I've completed my manual testing and confirm PROMPT_COMMAND looks as expected and preexec_functions and precmd_functions work as expected.

@NoahGorny
Copy link
Member

@cornfeedhobo I think we will first merge #1820 and then this, so prepare for a rebase 😄

@cornfeedhobo cornfeedhobo mentioned this pull request Feb 2, 2021
5 tasks
@buhl
Copy link
Contributor

buhl commented Feb 2, 2021

I like that it will be more explicit what we load when and nice catch with the loading order of preexec 👍

I would like to look at bash-it.sh at a later state to see if we can trim it down even further. I think we are loading or trying to load thinks that should maybe be loaded in a theme or plugin instead.

@cornfeedhobo cornfeedhobo force-pushed the cleanup-and-load-libraries-explicitly branch from 87db68b to e7fa160 Compare February 3, 2021 15:24
@cornfeedhobo
Copy link
Member Author

cornfeedhobo commented Feb 3, 2021

Github is having issues today. I might need to close and re-open this PR or push another branch, because I rebased and I'm not seeing the same diff this PR is reporting - it shouldn't be including reloader.bash.

@cornfeedhobo cornfeedhobo force-pushed the cleanup-and-load-libraries-explicitly branch from e7fa160 to ff2f961 Compare February 3, 2021 19:12
@cornfeedhobo cornfeedhobo deleted the cleanup-and-load-libraries-explicitly branch February 3, 2021 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants