RFR: 8231093: Document the ZIP FS properties noCompression and releaseVersion
Lance Andersen
lance.andersen at oracle.com
Wed Sep 25 18:23:57 UTC 2019
Hi Christoph
Thank you for the feedback, more below:
> On Sep 25, 2019, at 6:58 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
>
> Hi Lance,
>
> first of all, thanks for doing these updates to zipfs. It gets better and better ��
:-)
>
> I’ve been looking at the change now, too. Here are my findings:
>
> JarFileSystem.java:
> Line 96: remove
done
> Line 98: space should be between if and (, not after (.
done
> Line 103: there are quite some spaces (too much) between return and o
done
>
> Generally I’m wondering whether the classes JarFileSystem and JarFileSystemProvider couldn’t be removed at all and we could just add this multi-release implementation part into the ZipFileSystem class.
Well that is beyond the scope of this change :-)
> But that could be a subsequent change.
I was not involved, but it might have been discussed as part of http://hg.openjdk.java.net/jdk9/dev/jdk/rev/5cebf921be7a <http://hg.openjdk.java.net/jdk9/dev/jdk/rev/5cebf921be7a>
But yes, any changes should be handled separately
>
> ZipFileSystem.java:
> Line 92: private static final String COMPRESSION_METHOD = "compressionMethod";
> I’d prefer if we could start the property definition constants with PROP_, so, could you use PROP_COMPRESSION_METHOD? And probably also the constants in line 84-87 should be aligned to PROP_POSIX, PROP_DEFAULT_OWNER … (I was using OPT for “Option” when I created them but probably the word “property” makes more sense here).
Ok, I changed them to PROPERTY_ vs PROP_ including the POSIX properties
>
> Iine 94: public static final String DEFLATED_COMPRESSION_METHOD -> this should probably be private. I’d also rather use COMPRESSION_METHOD_DEFLATED
> line 96: public static final String STORED_COMPRESSION_METHOD -> same: should be private and COMPRESSION_METHOD_STORED
Changed the names and I intended to make them private originally but missed that when I refactored via intellij
> line 173: insert a space between use and (STORED or DEFLATED)
done
> line 177: Should be compatibility rather than compatability
done
> method getDefaultCompressionMethod (line 181 ff)
>
> I’d rather code it like:
> If (env.containsKey(COMPRESSION_METHOD)) {
> …
> } else {
> return isTrue(env, "noCompression") ? METHOD_STORED : METHOD_DEFLATED;
> }
>
I left it as it was as I think this comes to style choice
> Independent of that in lines 184, 186 and 187 you could insert a space after the if. That would match more with the style of the rest of the file. Also line 188, I’d prefer a space between switch and (
done
>
> Line 202: String.format is missing the argument COMPRESSION_METHOD.
done
>
> ZipFileSystemProvider.java:
> In line 57 and 60, I’d declare the constants rather package private than protected.
> Line 59: compatibility
done
> Line 126/127: You can just do “return getZipFileSystem(path, env);”
done
>
> module-info.java:
> line 241: start with a “be” -> will be considered…
done
>
> MultiReleaseJarTest.java:
> Line 132, 133: Add a space after the ,
done
>
> CompressionModeTest.java:
> You could remove the empty lines 153 and 170.
done
The revised webrev can be found at http://cr.openjdk.java.net/~lancea/8231093/webrev.03/index.html <http://cr.openjdk.java.net/~lancea/8231093/webrev.03/index.html>
Mach5 jdk tiers 1-3 are still clean
Thank you again.
Best
Lance
>
> Thanks & Best regards
> Christoph
>
>
> From: nio-dev <nio-dev-bounces at openjdk.java.net <mailto:nio-dev-bounces at openjdk.java.net>> On Behalf Of Lance Andersen
> Sent: Dienstag, 24. September 2019 01:42
> To: Alan Bateman <Alan.Bateman at oracle.com <mailto:Alan.Bateman at oracle.com>>
> Cc: nio-dev <nio-dev at openjdk.java.net <mailto:nio-dev at openjdk.java.net>>
> Subject: Re: RFR: 8231093: Document the ZIP FS properties noCompression and releaseVersion
>
> Please see http://cr.openjdk.java.net/~lancea/8231093/webrev.02/index.html <http://cr.openjdk.java.net/~lancea/8231093/webrev.02/index.html>
>
> As part of the latest update, I removed the documentation on specifying a Version Object for the “runtimeVersion” property.
>
> I did however leave the code in place for backward compatibility. I believe the rest of your input has been addressed.
>
> I also updated the CSR.
>
> Mach5 tiers1-3 run clean
>
> Best
> Lance
> On Sep 23, 2019, at 4:32 PM, Lance Andersen <lance.andersen at oracle.com <mailto:lance.andersen at oracle.com>> wrote:
>
> Thank you for the feedback Alan.
>
> More below
>
> On Sep 23, 2019, at 2:32 PM, Alan Bateman <Alan.Bateman at oracle.com <mailto:Alan.Bateman at oracle.com>> wrote:
>
> On 22/09/2019 23:43, Lance Andersen wrote:
>
> I updated the patch and the CSR.
>
> The updated webrev is: http://cr.openjdk.java.net/~lancea/8231093/webrev.01/index.html <http://cr.openjdk.java.net/~lancea/8231093/webrev.01/index.html>
> I went through the spec updates in module-info.java.
>
> "when adding entries ...". Is that also true when someone re-writes a file?
> Yes it does. I can change “adding” to “writing"
>
>
> Do we need quotes around "STORED" and "DEFLATED" value to make it clearer that they are strings?
> Sure I can add the quotes
>
>
> "an IllegalArgumentException will be thrown". This could be expanded to say that will be thrown when creating the file system.
> I can include the additional wording
>
>
> For releaseVersion there are 3 list items that start "If the value ..." and one that starts with "If the Object". We should keep the wording consistent. Also it feels like it accepts too many possible value types and maybe some of these should be dropped.
>
> Please see below WRT dropping Version
>
>
> Could we link "multi-release JAR" to the "Multi-release JAR files" section of the JAR file spec. I think{@docRoot}/../specs/jar/jar.html <mailto:%7b at docRoot%7d/../specs/jar/jar.html> should work.
>
> Sure I can add that…
>
>
> When the value of releaseVersion is a String or Integer then it's the feature version. In MR JARs the version is the major version of the Java platform release so "11.0.1" shouldn't be in the example.
> I can drop 11.0.1 from the example. Runtime.Version.parse() will accept “11.0.1” and return “11” which is the only reason I included the example . I can remove the “11.0.1” from the example.
>
>
> The paragraph for a value of "runtime" or Runtime.Version is a bit difficult to read. I think you want "runtime" to use Runtime.version().feature(). If a Runtime.Version object is provided then it's feature() method will be invoked. Maybe it would be better to split these out or maybe allowing a Runtime.Version object should be dropped to reduce the number of possible value types.
>
> I am more than happy to drop the Runtime.Version object. However as you are probably aware it was there previously and while it is probably highly unlikely that anyone is passing a Version object, if we drop it from the new”runtimeVersion” property, I assume we drop it from the “multi-release” property as well? I can update the CSR to indicate we are dropping Version.
>
> Best
> Lance
>
>
> -Alan
>
>
>
> <oracle_sig_logo.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>
>
>
>
>
> <image001.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>
>
>
>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190925/88c9199e/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oracle_sig_logo.gif
Type: image/gif
Size: 658 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190925/88c9199e/oracle_sig_logo-0001.gif>
More information about the nio-dev
mailing list