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

Installation path as an option #12

Merged
merged 2 commits into from
Jan 21, 2022
Merged

Installation path as an option #12

merged 2 commits into from
Jan 21, 2022

Conversation

Zettervall
Copy link
Contributor

I added install_path as an option to the install function to let the user be able to specify where njc should download and install the jre. I also had to add the installation path to both executeJar and execute-jar and execute-class-with-cp to be able to use the JRE installed in another location than default.
Default path is till __dirname.
My use case is that I want to be able to use NJC inside of a electron app. But the user is not allowed to access files inside of the electron package (__dirname).
In my solution installPath is passed around alot and needed for both install and execution. I could not come up with a more tidy solution but please have a look if you can figure out a way to reuse the install_path for both install and execution.

…or the download and installation of the JRE.
@NLueg
Copy link
Owner

NLueg commented Jan 20, 2022

Hey @Zettervall, thank you very much for your PR!
The change itself looks good to me. But can you have a look into the unit tests?
They're currently failing.

and made the existing test assert that
the default params generates a valid url
@Zettervall
Copy link
Contributor Author

@NLueg
Hi,
Thank you for the feedback. I'm kind of new to JS and TS (I'm a java developer) and I didn't realized there were any tests.
I have now fixed the failing test and since I moved were the call to "generateInstallOptions()" is done I also added an test to test that method. The old "default url for no parameters" test is now making sure that the default params can generate a valid URL if "getUrlToCall is passed with the default ones.

@NLueg
Copy link
Owner

NLueg commented Jan 21, 2022

No problem, thanks for your update! Looks good to me now.
I just published your change with version 1.1.0!

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

2 participants