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

Use bats_load_library instead of load and suggest to use BATS_LIB_PATH #10

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

brokenpip3
Copy link
Contributor

No description provided.

@brokenpip3 brokenpip3 added the wip label Aug 27, 2023
@brokenpip3 brokenpip3 changed the title Update bats-file to 0.4.0 and use bats_load_library instead of load Update bats-file to 0.4.0 and use bats_load_library instead of load for all the libraries Aug 27, 2023
Copy link
Member

@martin-schulze-vireso martin-schulze-vireso left a comment

Choose a reason for hiding this comment

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

I would not recommend to change the lib path in setup.

@brokenpip3
Copy link
Contributor Author

I would not recommend to change the lib path in setup.

thanks for the review, you mean only in the brew formula or in general in the instructions for the user?

@martin-schulze-vireso
Copy link
Member

I mean in general. The idea of the lib path is to separate system config from the test suite sources. If you hardcode brew paths, what happens when the tests are run in a system without homebrew?

@brokenpip3
Copy link
Contributor Author

brokenpip3 commented Aug 28, 2023

If you hardcode brew paths, what happens when the tests are run in a system without homebrew?

I totally understand your concern but this is a homebrew packages for mac-os users right? I mean -we could but- we never added the linux support to these packages (again it's possible but it will require using brew in linux which is something very few people do for obvious reasons)

Also the BATS_LIB_PATH="${BATS_LIB_PATH}:${HOMEBREW_PREFIX}/lib" will add to an existing BATS_LIB_PATH variable another path that could be even empty or not valid right?

If I have on my bash profile this config in my linux box and I do not have brew installed:

BATS_LIB_PATH="/usr/lib/bats"

and I will use the previous export command with the homebrew prefix the tests will run correctly on both my linux and mac box right?
I'm probably missing something here :)

Anyway I'm totally ok to remove the lib path export, what about keeping the bats_load_library instead of load? should be the preferred way of loading the libraries right?

@brokenpip3 brokenpip3 changed the title Update bats-file to 0.4.0 and use bats_load_library instead of load for all the libraries Use bats_load_library instead of load for all the libraries Nov 4, 2023
@brokenpip3 brokenpip3 changed the title Use bats_load_library instead of load for all the libraries Use bats_load_library instead of load and suggest to use BATS_LIB_PATH Nov 4, 2023
@brokenpip3
Copy link
Contributor Author

I created a separated PR to accommodate both file and detik update: #11 and changed the title of this cosmetic one to be threaded a separate change so it can attract more brew users feedback (since I'm not).

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.

2 participants