RFR: 8231093: Document the ZIP FS properties noCompression and releaseVersion

Fri Sep 27 10:21:08 UTC 2019

Hi Lance,

thanks for updating the webrev. It looks good now.

Regarding refactoring the JarFileSystem/JarFileSystemProvider classes, I plan to have a look at after this change got pushed.

Best regards
Christoph

> -----Original Message-----
> From: Lance Andersen <lance.andersen at oracle.com>
> Sent: Mittwoch, 25. September 2019 20:24
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: nio-dev <nio-dev at openjdk.java.net>
> Subject: Re: RFR: 8231093: Document the ZIP FS properties noCompression
> and releaseVersion
> 
> Hi Christoph
> 
> Thank you for the feedback, more below:
> 
> On Sep 25, 2019, at 6:58 AM, Langer, Christoph
> <mailto: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
> 
> 
> 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
> 
> Mach5 jdk tiers 1-3 are still clean
> 
> Thank you again.
> 
> Best
> Lance
> 
> 
> Thanks & Best regards
> Christoph
> 
> 
> From: nio-dev <mailto:nio-dev-bounces at openjdk.java.net> On Behalf
> Of Lance Andersen
> Sent: Dienstag, 24. September 2019 01:42
> To: Alan Bateman <mailto:Alan.Bateman at oracle.com>
> Cc: nio-dev <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
> 
> 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
> <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
> <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
> 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
> thinkmailto:%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
> 
> 
> 
> http://oracle.com/us/design/oracle-email-sig-198324.gif
> http://oracle.com/us/design/oracle-email-sig-198324.gifLance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> 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.gifLance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> 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.gifLance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> mailto:Lance.Andersen at oracle.com
> 
> 
> 