-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(#5): Add java
and javac
methods to Jhome
#6
Conversation
@yegor256 Could you take a look, please? |
@volodya-lombrozo in case of Java8 it's JRE, not JDK |
@yegor256 Take a look one more time, please |
* @return The path of it | ||
*/ | ||
public Path java() { | ||
return this.home.resolve("bin").resolve("java"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@volodya-lombrozo looks like code duplication to me. It's better to call path("bin/java")
from here
* {@code Optional<Path>} is also a solution here. | ||
*/ | ||
public Path javac() { | ||
return this.home.resolve("bin").resolve("javac"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@volodya-lombrozo same here, looks like code duplication
@volodya-lombrozo I made it work on Windows (see the changes in the test). Would be great if in your pull request you also update README and explain that calling |
@yegor256 Updated the PR according with your suggestions. Could you take a look one more time please? |
@volodya-lombrozo the tests don't pass on windows ( |
@yegor256 I fixed the problem with the Windows OS. Could you take a look, please? |
@rultor merge |
Add
java
andjavac
methods toJhome
.Closes: #5.