Update CcmBridge#getCurrentJvmMajorVersion to use regular expressions #1738
Conversation
absurdfarce left a comment
absurdfarce left a comment There was a problem hiding this comment.
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?
| */ | ||
| private static int getCurrentJvmMajorVersion() { | ||
| @VisibleForTesting | ||
| static int getCurrentJvmMajorVersion() { |
There was a problem hiding this comment.
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]+)\\..*$"); |
There was a problem hiding this comment.
Nit: I don't think the "$" is doing much for you here
| Note: Jenkins failures are entirely derived from the switch to the ASF repo |
hhughes force-pushed the refactor_jvm_detection_helper branch from aee5f29 to cd75885 Compare 2 participants
No description provided.