RFR 8211919: ZipDirectoryStream should provide a stream of paths that are relative to the directory
lance.andersen at oracle.com
Mon Jan 14 18:46:19 UTC 2019
Thank you for the feedback, please see below
> On Jan 14, 2019, at 9:13 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
> Hi Lance,
> as I'm currently active in that area, it's an easy one for me to review this
> Overall the change looks good, thanks for doing it. Here are some few nits I discovered:
> in the comments:
> line 426: Maybe you could take the chance to improve the text of the comment ?
> // (1) assume all path from zip file itself is "normalized" -> (1) assume each path from zip file is "normalized"
> line 432: I think the word "also" can be removed
> line 95: Could you please also fix the indentation of @Override (4 instead of 3 spaces)
> line 45: I think you should remove 8211385 from the @bug tag, since you removed the DirectoryStream tests from this file
> I think you should add a @bug tag for both, 8211385 and 8211919
> line 53: UNZIPFS_MAP: not needed
I removed it though I had added it as I have some additional tests to add later which will need it.
> line 70: ; not needed in try statement
done (thought I got all of these earlier, guess not)
> line 72: jarFile should be createad outside of try block
Utils.createJarFile throws an IOException which is why it is there vs introducing a 2nd try block.
> line 97: spelling: “filter”
fixed though was “DirectoryStream.Filter” which I changed to DirectoryStream filter
> line 106: space before + missing
> line 144: ";" is not needed and bracket of try could be closed on this line
> line 170: Spelling: use "a" instead of "an" ?
> line 187: "an" instead of "a" ?
> line 205: same ("an" instead of "a”)
“a” vs "an" is always fun, should be addressed.
> line 215: the assert for IllegalStateException could be dropped here, as it is a superclass of ClosedDirectoryStreamException which is asserted in line 219? Unless you want to use this as a test for ClosedDirectoryStreamException class hierarchy…
Well the spec expects IllegalStateException. ClosedDirectoryStreamException is thrown by ZipFS so I also included this for now as a sanity check in case someone was already catching this. I am happy to remove it though
> line 238: I think this commented debug code should be removed.
done, thought I did that earlier
> line 306: Looks like the matcher instance is not needed
true and removed
> line 309: you could use an import java.util.zip.ZipException?
done, still getting used to Intellij vs Netbeans which I like better for import handling
> line 314: The check should rather be: if (ds.iterator().hasNext()) throw…
The check is fine as it is but I changed it to make it clearer similar to DirectoryStream/Basic.java
Again, thank you for your feedback.
Updated webrev is here http://cr.openjdk.java.net/~lancea/8211919/webrev.01/index.html
> Best regards
>> -----Original Message-----
>> From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> On Behalf
>> Of Lance Andersen
>> Sent: Samstag, 12. Januar 2019 21:13
>> To: core-libs-dev <core-libs-dev at openjdk.java.net>; nio-dev <nio-
>> dev at openjdk.java.net>
>> Subject: RFR 8211919: ZipDirectoryStream should provide a stream of paths
>> that are relative to the directory
>> Hi all,
>> The following patch addresses the issue where the stream of paths where
>> not relative to the specified directory as needed.
>> As part of the fix, I added additional DirectoryStream tests similar to the tests
>> in nio/file/DirectoryStream/Basic.java.
>> Webrev can be found:
>> <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>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>
More information about the core-libs-dev