-
Notifications
You must be signed in to change notification settings - Fork 543
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 convinient method to check for the currently used Java Version #120
Conversation
ff951f9
to
70ffbd4
Compare
|
||
public static void assumeJavaVersion(JavaVersion version) | ||
{ | ||
assumeThat( "java.specification.version: ", |
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.
Nice PR. Maybe SystemUtils.JAVA_SPECIFICATION_VERSION + ":
instead. Do you like static imports?
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.
Hey Tibor,
do you mean to use a constant instead of the string in the message part of assumeThat
? There is no constant for the property name in Commons Lang. But I could introduce one in HelperAssertions
or we could extend SystemUtils, but then we have to wait for the next release of Commons Lang.
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.
I've added the static import.
8a854ef
to
498942b
Compare
Hello @Tibor17 I've addressed you're comment about comparing Strings. There already is an API for comparing JavaVersions in SystemUtils. Hope you like this. |
assumeThat( "java.specification.version: ", | ||
JAVA_SPECIFICATION_VERSION, is( greaterThanOrEqualTo( version.toString() ) ) ); | ||
assumeTrue( "java.specification.version: " + JAVA_SPECIFICATION_VERSION, | ||
SystemUtils.isJavaVersionAtLeast( version ) ); |
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.
Yes this is cool. Thx.
I will come back to your PRs soon.
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.
I am running the build mvn install -Prun-its, let's wait for the result.
@britter |
@britter |
@Tibor17 thank you for integrating this. Did you know, that it is possible to mark PRs as merged in GitHub? I've documented the workflow in the Apache Commons Wiki. |
I don't see such option in GUI. Is not it because of we are not the owner of this repo |
No description provided.