On Tue, 13 May 2025 12:32:56 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix comment based on current behaviour.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 147:
>
>> 145: // this could exist somewhere common, but if it's definitely never
>> going to
>> 146: // be shared, it could be made public here.
>> 147: private enum AccessMode {
>
> Hello David, I think having it `private` is fine and in fact more
> appropriate. I would suggest removing the comment though.
Done. And if it's not getting made visible (i.e. values are always strings) I
can simplify things too.
> src/jdk.zipfs/share/classes/module-info.java line 277:
>
>> 275: * <td>
>> 276: * A value defining the desired read/write access mode of the
>> file system
>> 277: * (either <em>read-write</em> or <em>read-only</em>).
>
> This line is slightly confusing. Initially I thought `read-write` and
> `read-only` are the actual values that this environment property will accept.
> But further review suggests that the actual literals supported are `readOnly`
> and `readWrite` and this line here I think is trying to explain the supported
> semantics of the file system.
>
> Perhaps we could reword this to something like:
>
>>
>> A value defining the desired access mode of the file system. A ZIP file
>> system can be created to allow for read-write access or read-only access.
Yeah, I tried to distinguish the accessMode flags (for which there are 3
states, ro, rw, n/a) and the final state of the file system from
fs.isReadOnly(). Hence using `<em>...</em>` for whenever the actually resulting
state is mentioned. I guess it wasn't clear enough.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091645807
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2091649006
