[MSHARED-1416] - Review and improve the term "Jdk revision" - #129
[MSHARED-1416] - Review and improve the term "Jdk revision"#129belingueres wants to merge 1 commit into
Conversation
- renamed jdkRevision to maxJavaClassVersion. - deprecated jdkRevision's getter & setter.
| * The highest JVM revision available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM that guarantees compatibility. | ||
| * The highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM version that guarantees compatibility. |
There was a problem hiding this comment.
so is it JVM version or Java byte code version?
There was a problem hiding this comment.
I really doubt that's right. If it is, then the field is incorrectly named. One of them is certainly wrong.
There was a problem hiding this comment.
The javadoc comment most probably is confusing. How about this comment:
/**
* The highest Java version required by any class in the JAR (e.g. "17" if the newest
* class file version present corresponds to Java 17).
* <p>
* Because this is the highest requirement across all classes, it also serves as the
* minimum JVM version needed to safely run the entire JAR. The JAR may still happen
* to run on an older JVM if the classes requiring this version are never invoked,
* but that isn't guaranteed.
*/
There was a problem hiding this comment.
If this is right, the field name is wrong. I'm prettys sure the field name is wrong whether this is right or not.
There was a problem hiding this comment.
The field name is the one agreed in the issue's discussion (#100)
There was a problem hiding this comment.
That doesn't mean the field name is correct. And the docs are still inconsistent.
| * | ||
| * @since 3.2.1 | ||
| */ | ||
| public void setMaxJavaClassVersion(String maxJavaClassVersion) { |
There was a problem hiding this comment.
why do we need a setter at all?
There was a problem hiding this comment.
Currently it is the only way to set a value to the variable.
| * The highest JVM revision available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM that guarantees compatibility. | ||
| * The highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM version that guarantees compatibility. |
There was a problem hiding this comment.
I really doubt that's right. If it is, then the field is incorrectly named. One of them is certainly wrong.
| /** | ||
| * The highest JVM revision available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM that guarantees compatibility. | ||
| * The highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular |
There was a problem hiding this comment.
I don't think "Java class version" is a thing. This also needs to be very clear about whether you're talking about the class file format version or the Java version
| * The highest JVM revision available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM that guarantees compatibility. | ||
| * The highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM version that guarantees compatibility. |
There was a problem hiding this comment.
If this is right, the field name is wrong. I'm prettys sure the field name is wrong whether this is right or not.
| * The highest JVM revision available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM that guarantees compatibility. | ||
| * The highest Java class version available in any class files. While the JAR may work on earlier JVMs if particular | ||
| * classes are not used, this is the minimum JVM version that guarantees compatibility. |
There was a problem hiding this comment.
That doesn't mean the field name is correct. And the docs are still inconsistent.
2 participants
Fixes #100 : [MSHARED-1416] - Review and improve the term "Jdk revision"
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.