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

Vendoring the composure #1820

Merged
merged 3 commits into from
Feb 3, 2021
Merged

Vendoring the composure #1820

merged 3 commits into from
Feb 3, 2021

Conversation

buhl
Copy link
Contributor

@buhl buhl commented Jan 28, 2021

Description

As per #1818
This still needs a little love.
I had to move up the loading of vendored libs because composure was loaded as the first thing in bash_it.sh
This means I can't use __log_debug when loading vendored libs. A decision has to be made on what we do with this issue.

Also my linting has started to replace spaces with tabs in files. I am not sure if this is an error on my part, if it is, I need some help to fix it. So this PR looks bigger than it had to be.

Once we settle on what to do with the logging I will finish adding files to clean_files and such.

Types of changes

Moved the composure library to the vendor folder

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.

@buhl
Copy link
Contributor Author

buhl commented Jan 28, 2021

hmm, the tests are failing. I had to add the .bash extension locally to make them work. It seems like the CI test would like me to remove them?

@buhl
Copy link
Contributor Author

buhl commented Jan 28, 2021

This bats loading thing got a little messy. I will have to clean-up/rebase the branch before it's ready for a merge.
I don't get why I sometimes need the .bash ext on load in bats files and sometimes don't.
also the check for bad/missing shellcheck header is a little annoying when a file might need a she-bang header 😅
@NoahGorny This is ready for a preliminary review.

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Jan 28, 2021

There is a lot that I'd like to change and I think a full review would get messy. I might make a branch, but might resort to a bunch of comments...

Edit: glad you isolated this to it's own PR though 😆

@buhl
Copy link
Contributor Author

buhl commented Jan 29, 2021

@cornfeedhobo

Yeah, I consider the PR a POC for now.
It was a pretty big rewrite so I am ready for many comments/ideas/opinions.
I am in no way married to how this PR turned out. So comments or other branches are welcome 😊
It's also a good idea for some maintainers to try using 'git vendor' before many more things are vendored 😊

@NoahGorny
Copy link
Member

Hey @buhl , well done!

I think that an easier solution would be to source composure just like we did before, and make lib/composure.bash just load the correct file/symlink. Alternatively, we can just load the init.d/composure script directly at the start, in any case, this will enable us to continue logging and avoid major changes to the way we load components

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Jan 29, 2021

Alternatively, we can just load the init.d/composure script directly at the start

This is where I would go with this. I actually want to kill the loop loader and explicitly source libraries, in order, anyways. We don't have so many that the list would be unwieldy, so I've come to see the loop as an unneeded abstraction.

Edit: Also I would just source from vendor and scrap the init.d. If we really wanted to keep the init.d convention, then I think files there should be prefixed with a number to guarantee load order.

@buhl
Copy link
Contributor Author

buhl commented Jan 29, 2021

I like these solutions. I will refactor the load of composure in this PR to just source the file directly from the vendor folder in the top of bash_it.sh this also correspond well with what as discussed here #1818 (comment)
I will make another PR doing the same for preexec to keep things separate. That PR would then also remove the loop.

Another function the files in vendor/init.d had was to give a place where we could and some extra functions/settings around a vendored lib. This functionality we could move to a file in the lib/ folder if needed.

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 some comments, but this seems like a pretty clean and good change! nice work!

install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
bash_it.sh Show resolved Hide resolved
install.sh Show resolved Hide resolved
@buhl
Copy link
Contributor Author

buhl commented Jan 29, 2021

I still need to make sure files are added to clean_files.txt and the branch needs a little rebasing - to many pointless commits.
I will do these last steps once everything else is ready for merge.

…3698d

git-subtree-dir: vendor/github.com/erichs/composure
git-subtree-split: 5c3698df33cf92f9dbe75b807b1d29729989baaa
git-vendor-name: composure
git-vendor-dir: vendor/github.com/erichs/composure
git-vendor-repository: https://github.com/erichs/composure
git-vendor-ref: 1.3.1
@buhl
Copy link
Contributor Author

buhl commented Jan 31, 2021

I have rebased and forced pushed this branch. If the changes can be accepted it's ready for merge

Also by merging this PR #872 can be closed I think.

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 @buhl
I have left some small comments, I will look again after you will address them 😄

bash_it.sh Outdated Show resolved Hide resolved
bash_it.sh Outdated Show resolved Hide resolved
@cornfeedhobo
Copy link
Member

@buhl I'm going to make that branch when I have free time today, but you can also look at #1823 to get a sense of what I was meaning about loading in order, explicitly.

Fixed tests
Fixed install.sh and bash_it.sh
Added gitattributes to the vendor folder
Changed documentation
@buhl
Copy link
Contributor Author

buhl commented Feb 2, 2021

I fixed the last two comments and force pushed the branch.

@buhl buhl requested a review from NoahGorny February 2, 2021 15:44
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.

LGTM, well done @buhl !!

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.

@buhl I think this looks great! @NoahGorny great CR. We should definitely get this work merged and deal with any tinkering in follow up PRs.

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.

None yet

3 participants