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

Update CcmBridge#getCurrentJvmMajorVersion to use regular expressions #1738

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

hhughes
Copy link
Contributor

@hhughes hhughes commented Oct 30, 2023

No description provided.

Copy link
Contributor

@absurdfarce absurdfarce left a comment

Choose a reason for hiding this comment

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

I'm the first guy to applaud the use of regexes for... well, pretty much anything really. And I like the use of named capture groups here. But I do kinda wonder if we can't do the same kinda thing with just the following:

  • Splitting based on dot
  • Dropping a leading "1" if if's the first item in the result
  • Returning the first item after the drop
  • Throwing an exception if we don't find any dots

Am I missing an obvious case that makes that fall down?

@@ -430,17 +433,15 @@ private static File createTempStore(String storePath) {
*
* @return major version of current JVM
*/
private static int getCurrentJvmMajorVersion() {
@VisibleForTesting
static int getCurrentJvmMajorVersion() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'm kinda wondering if instead of making this visible for testing we don't just change things so that all the logic lives in a getCurrentJvmMajorVersion() impl which takes a version string as an arg:

private static int getCurrentJvmMajorVersion() {
   return getJvmMajorVersion(System.getProperty("java.version"));
}

static int getJvmMajorVersion(String versionSysprop) {
   // Same logic in here (without the sysprop access)
}

Would probably simplify your tests quite a bit.

if (dot != -1) {
version = version.substring(0, dot);
}
Pattern pattern = Pattern.compile("^(1\\.)?(?<major>[0-9]+)\\..*$");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I don't think the "$" is doing much for you here

@absurdfarce
Copy link
Contributor

Note: Jenkins failures are entirely derived from the switch to the ASF repo

@hhughes hhughes force-pushed the refactor_jvm_detection_helper branch from aee5f29 to cd75885 Compare November 3, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants