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

Add dev.sh to avoid setting GEM_HOME and GEM_PATH #487

Closed
wants to merge 1 commit into from

Conversation

k0kubun
Copy link

@k0kubun k0kubun commented Jan 18, 2023

Closes #422

I added dev.sh file as per #422 (comment).

Copy link
Owner

@postmodern postmodern left a comment

Choose a reason for hiding this comment

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

Spotted a bug on line 59. I also do not think dev.sh is necessary if it only sets a global variable.

Also, I am hesitant about adding more global variables. I think hook functions might be a better option. I may try to make a separate branch of the 0.4.0 branch to demonstrate the idea of extracting the GEM_HOME/GEM_PATH logic into a hook function that can be overrode.

share/chruby/chruby.sh Outdated Show resolved Hide resolved
share/chruby/dev.sh Outdated Show resolved Hide resolved
Copy link
Owner

@postmodern postmodern left a comment

Choose a reason for hiding this comment

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

Now this obviously needs tests. The unit tests are located in the test/unit/ directory and use shunit2 which you should be able to install via your system's package manager (brew install shunit2 or sudo apt install -y shunit2). You can probably copy the existing tests from test/unit/chruby_use_test.sh but test that GEM_HOME and GEM_PATH do not get set. The test files can be ran locally by simply executing the file, since the *_test.s files are just shell scripts.

@k0kubun
Copy link
Author

k0kubun commented Jan 20, 2023

Thanks for the advice. I copied test/unit/chruby_use_test.sh to test/unit/chruby_dev_test.sh and modified it for the changed behavior. I'd appreciate another look when you have a chance.

@postmodern
Copy link
Owner

The tests all passed! I will have to manually merge this into the 0.4.0 branch. Then I will have to start working on release engineering and preparing a 0.4.0 release. I may also need to do a 0.3.9 release since the main branch contains a fix for detecting mruby ruby versions.

@postmodern postmodern added this to the 0.4.0 milestone Jan 21, 2023
@postmodern
Copy link
Owner

Manually merged into 0.4.0.

@postmodern postmodern closed this Jan 21, 2023
@postmodern postmodern self-assigned this Jan 21, 2023
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.

A way to not set GEM_HOME if the installed ruby is under $HOME
2 participants