SI-7265 Buy some time to fix s.u.Properties.isJavaAtLeast#2580
SI-7265 Buy some time to fix s.u.Properties.isJavaAtLeast#2580soc wants to merge 1 commit intoscala:2.10.xfrom soc:SI-7265-temporary-fix
Conversation
There was a problem hiding this comment.
Isn't this all a bit roundabout?
def isJavaAtLeast(version: String) = version.length > 2 && version.startsWith("1.") && javaVersion(2) >= version(2)
There was a problem hiding this comment.
yes, one could imagine doing a bit better, especially since 1.10 is around the corner...
There was a problem hiding this comment.
It is seeking a middle ground with the previous attempt that compared the entire version string using bit masking; the details escape me. I've forgotten why split was deemed too high-level?
On simplicity: I wouldn't mind an API that asked
isJavaAtLeast(major: Int, minor: Int = 0, epoch: Int = 1) and isJavaAtLeast(5).
That jibes with the Scala ethos of not buying into every historical fiction in Java.
And no one will complain that their "1.7.0_36b3" lied to them.
+1 for leaving our grandchildren out of it, who may very well prefer the term transcircuitous to cyborg; and also omitting any reference to Gosling's Law, that the rate of progress of major versions is inversely proportional to the square of the sum of LOC + features + open bugs.
There was a problem hiding this comment.
that compared the entire version string using bit masking
Wow ... I just used some while loops! :-)
|
"I've forgotten why split was deemed too high-level?" split is the same as using a regex, and apparently regexes drag in a lot. |
|
Going to close this PR this week unless it gets updated. It's had pending review comments for over 10 days. |
|
@adriaanm Sorry, broken nose, hospital, etc... Reading through the comments, I don't see stuff which would be applicable to the issue here. (I sympathize with the version which takes integers instead of a string, but that doesn't really fix the issues with the string-based one, does it?) |
|
@soc, sorry to hear it! Hope it's healing well! |
|
I'm still in favor of my oneliner as opposed to the convoluted status quo (before this PR). |
|
@soc my condolences, too. I guess you have to be pretty careful how you give feedback on github, you never know how it will be received. I'd also suggest preferring one-line to involution or convolution. The one line does give different answers for |
|
The thing is that if you want to make that one-liner work reliably, you end up with exactly the change which was rejected before. So we have either
|
|
Here is something between a 1-liner, the 3-liner requested by @paulp and the original forty-niner I mean forty-liner. https://gist.github.com/som-snytt/5725336 There's a shorter take that assumes major.minor, and a longer version that takes as many parts as you want to try to compare against whatever the actual version string is. I preserve the original strategy of taking the numeric prefix of the part for purposes of comparison, so "0_52" is 0, "5b" is 5, "b" is 0. I think somewhere in the repo is version munging logic for the sbt build, or similar endeavor. |
|
I don't want to imagine how many classes this creates for something as trivial as comparing a version number ... |
|
Hey everyone, what are we doing? |
A little spec humo(u)r. |
|
So @soc would you like to take a shot at this based on java.specification.version, which makes it easy unless you're trying to future-proof against java 10.x. |
|
@soc: what's the status of this PR? |
|
@gkossakowski Without @paulp 's help we can't figure out how to perform the calculation in acceptable time and space such that the code is maintainably write-once read-many! But maybe @soc will bounce back. Maybe there's a scala.reflect.internal.util.ReflectionUtils.isJavaAtLeast method that could be made public. |
|
Not sure about a good solution ... I'll close it and will reopen if I or someone else has a new idea. |
|
Wait, isn't this just the temporary fix? |
|
I'll have a PR in a minute after I figure out how to submit against 2.10 under the new github interface, argh. The PR includes a junit test! ha! That's great. And corrects the scaladoc in this PR, which is backward. 🎱 I'm not sure I'm using 8ball correctly, but github suggested it. I kind of want my old github back. |
No description provided.