Hi Christoph
Thank you for the feedback, please see below
> On Jan 14, 2019, at 9:13 AM, Langer, Christoph <christoph.lan...@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:
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java:
> 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
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipPath.java:
> line 95: Could you please also fix the indentation of @Override (4 instead of
> 3 spaces)
>
done
> test/jdk/jdk/nio/zipfs/Basic.java:
> line 45: I think you should remove 8211385 from the @bug tag, since you
> removed the DirectoryStream tests from this file
done
>
> test/jdk/jdk/nio/zipfs/ZipFsDirectoryStreamTests.java
> I think you should add a @bug tag for both, 8211385 and 8211919
done
> 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
fixed
> line 144: ";" is not needed and bracket of try could be closed on this line
fixed
> 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
Lance
>
> Best regards
> Christoph
>
>> -----Original Message-----
>> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> On Behalf
>> Of Lance Andersen
>> Sent: Samstag, 12. Januar 2019 21:13
>> To: core-libs-dev <core-libs-dev@openjdk.java.net>; nio-dev <nio-
>> d...@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://cr.openjdk.java.net/~lancea/8211919/webrev.00/
>>
>> Best
>> Lance
>> <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.ander...@oracle.com <mailto:lance.ander...@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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
