RFR: 8231093: Document the ZIP FS properties noCompression and releaseVersion
Lance Andersen
lance.andersen at oracle.com
Fri Sep 27 11:29:56 UTC 2019
Hi Christoph,
> On Sep 27, 2019, at 6:21 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
>
> Hi Lance,
>
> thanks for updating the webrev. It looks good now.
Thank you. Once the CSR is blessed, I will push.
>
> Regarding refactoring the JarFileSystem/JarFileSystemProvider classes, I plan to have a look at after this change got pushed.
OK, I am hoping Alan or Sherman(if he is reading this alias :-) ) might have some of the history.
Thank you again!
Best
Lance
>
> 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
>>
>>
>>
>
<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/20190927/99227949/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/20190927/99227949/oracle_sig_logo-0001.gif>
More information about the nio-dev
mailing list