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

Preexec as an updatedable library #1587

Closed
wants to merge 3 commits into from
Closed

Preexec as an updatedable library #1587

wants to merge 3 commits into from

Conversation

buhl
Copy link
Contributor

@buhl buhl commented May 9, 2020

As per #1304 (comment)
I know it's not ready to merge but I thought we could have a discussion on how we would go about including libraries into bash-it

Personally I find git submodules a total nogo. They are a pain to work with especially if you start to have more than a few. Git subtress could work, but I think, for now, a simple Makefile (as in my PR) or a bash script will do.

We might also have to reconsider how we load these libraries into bash-it

buhl added 3 commits January 7, 2021 23:32
Makefile to update vendors should maybe be a bash script. it is
however only ment to be run by developers.

Maybe vendors should be a git-subtree(1) instead Maybe we should
always supply a bash-it-init.bash(or some other name) file to the
vendoer forlder so we have a standardized way of loading vendor libs
from bash_it.sh
@buhl
Copy link
Contributor Author

buhl commented Jan 7, 2021

I just synced my PR with the main branch. I have been using this for over an year now with no problems.

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Jan 7, 2021

Thanks for your PR! You've got some good stuff in this PR. Yeah, let's discuss.

So far, I like the idea of vendors/ because like you said, submodules are usually a no-go. I'm going to think about this for another day while giving some time for others to weigh in...

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.

I left a couple of comments that I think should be addressed, thanks for working on it @buhl !

@egrep "^[0-9a-zA-Z\-]+:$$" Makefile | grep -v "help:"

update-preexec:
# - Maybe we should make this into a git-subtree(1) instead
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is on point- we should start using git-subtree for this kind of stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might have another idea I want to try before resorting to subtrees as they can also get a bit messy to work with.


update-preexec:
# - Maybe we should make this into a git-subtree(1) instead
# - Maybe we should always supply a bash-it-init.bash file to the vendoer forlder so we have a
Copy link
Member

Choose a reason for hiding this comment

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

I also agree with this comment, this will make the loading way more clear and rebust

# See "${BASH_IT}/vendors/Makefile for comments
for _bash_it_config_file in ${BASH_IT}/vendors/*/*.bash
do
source "$_bash_it_config_file"
Copy link
Member

Choose a reason for hiding this comment

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

you should add _log_debug here

@buhl buhl mentioned this pull request Jan 8, 2021
6 tasks
@buhl
Copy link
Contributor Author

buhl commented Jan 8, 2021

I wanted to try out something a little different than this PR. So I have made a new one #1776
I thought it would be easiest to have a discussion on that particular idea in a separate PR.
We can just close the PR if it's too invasive.

@NoahGorny
Copy link
Member

I think we have settled on #1776 being the better solution 😄

@NoahGorny NoahGorny closed this Jan 16, 2021
@buhl buhl deleted the preexec branch January 17, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants