Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Thu, 27 Jun 2024 10:27:03 GMT, Aleksei Efimov wrote:
>> Nizar Benalla has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - use javadoc standards
>> - Merge remote-tracking branch 'upstream/master' into naming
>> - remove whitespace
>> - 8332072: Convert package.html files in `java.naming` to package-info.java
>
> src/java.naming/share/classes/javax/naming/ldap/spi/package-info.java line 29:
>
>> 27: *
>> 28: * Extends the {@code javax.naming.ldap} package to provide the classes
>> 29: * and interfaces for the Service Provider Interface (SPI) for LDAP
>
> The `javax.naming.ldap.spi` package only contains SPI classes that can
> customize DNS lookups when performing LDAP operations. It was added by
> [JDK-8160768](https://bugs.openjdk.org/browse/JDK-8160768)/[CSR](https://bugs.openjdk.org/browse/JDK-8192975).
>
> Since the package only contains classes related to this SPI maybe we could
> change the wording to something like this:
> Suggestion:
>
> * Provides the Service Provider Interface for DNS lookups when
> * performing LDAP operations.
Thanks, Fixed in
[0104e4a](https://github.com/openjdk/jdk/pull/19529/commits/0104e4ac5a9bfe143822e72756771c29e0f32731)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1658495820
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Wed, 26 Jun 2024 20:41:56 GMT, Jonathan Gibbons wrote:
>> src/java.naming/share/classes/javax/naming/ldap/package-info.java line 200:
>>
>>> 198: * if (respCtls != null) {
>>> 199: * // Find the one we want
>>> 200: * for (int i = 0; i < respCtls.length; i++) {
>>
>> This is the only change that isn't a direct 1:1 conversion, I hope this ok.
>
> This seems reasonable for what is an obvious but minor bug
The change looks correct to me too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1656893835
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Wed, 26 Jun 2024 18:11:45 GMT, Nizar Benalla wrote:
>> Can I please get a review for this small change? The motivation is that
>> javac does not recognize `package.html` files.
>>
>> The conversion was simple, I used a script to rename the files, append "*"
>> on the left and remove some HTML tags like `` and ``. I did the
>> conversion in place, renaming them in git but with the big amount of change
>> `git` thinks it's a new file.
>>
>> I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I
>> hope that's fine.
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains four additional
> commits since the last revision:
>
> - use javadoc standards
> - Merge remote-tracking branch 'upstream/master' into naming
> - remove whitespace
> - 8332072: Convert package.html files in `java.naming` to package-info.java
src/java.naming/share/classes/javax/naming/ldap/spi/package-info.java line 29:
> 27: *
> 28: * Extends the {@code javax.naming.ldap} package to provide the classes
> 29: * and interfaces for the Service Provider Interface (SPI) for LDAP
The `javax.naming.ldap.spi` package only contains SPI classes that can
customize DNS lookups when performing LDAP operations. It was added by
[JDK-8160768](https://bugs.openjdk.org/browse/JDK-8160768)/[CSR](https://bugs.openjdk.org/browse/JDK-8192975).
Since the package only contains classes related to this SPI maybe we could
change the wording to something like this:
Suggestion:
* Provides the Service Provider Interface for DNS lookups when
* performing LDAP operations.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1656892104
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Wed, 26 Jun 2024 18:08:21 GMT, Nizar Benalla wrote:
>> Nizar Benalla has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - use javadoc standards
>> - Merge remote-tracking branch 'upstream/master' into naming
>> - remove whitespace
>> - 8332072: Convert package.html files in `java.naming` to package-info.java
>
> src/java.naming/share/classes/javax/naming/ldap/package-info.java line 200:
>
>> 198: * if (respCtls != null) {
>> 199: * // Find the one we want
>> 200: * for (int i = 0; i < respCtls.length; i++) {
>
> This is the only change that isn't a direct 1:1 conversion, I hope this ok.
This seems reasonable for what is an obvious but minor bug
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1655501908
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
> Can I please get a review for this small change? The motivation is that javac > does not recognize `package.html` files. > > The conversion was simple, I used a script to rename the files, append "*" on > the left and remove some HTML tags like `` and ``. I did the > conversion in place, renaming them in git but with the big amount of change > `git` thinks it's a new file. > > I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I > hope that's fine. Nizar Benalla has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - use javadoc standards - Merge remote-tracking branch 'upstream/master' into naming - remove whitespace - 8332072: Convert package.html files in `java.naming` to package-info.java - Changes: - all: https://git.openjdk.org/jdk/pull/19529/files - new: https://git.openjdk.org/jdk/pull/19529/files/768eebf5..1826633f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19529&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19529&range=00-01 Stats: 67137 lines in 1456 files changed: 42388 ins; 18751 del; 5998 mod Patch: https://git.openjdk.org/jdk/pull/19529.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19529/head:pull/19529 PR: https://git.openjdk.org/jdk/pull/19529
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Wed, 26 Jun 2024 18:09:10 GMT, Nizar Benalla wrote:
>> Can I please get a review for this small change? The motivation is that
>> javac does not recognize `package.html` files.
>>
>> The conversion was simple, I used a script to rename the files, append "*"
>> on the left and remove some HTML tags like `` and ``. I did the
>> conversion in place, renaming them in git but with the big amount of change
>> `git` thinks it's a new file.
>>
>> I also added a new `package-info.java` file to `javax.naming.ldap.spi`. I
>> hope that's fine.
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains four additional
> commits since the last revision:
>
> - use javadoc standards
> - Merge remote-tracking branch 'upstream/master' into naming
> - remove whitespace
> - 8332072: Convert package.html files in `java.naming` to package-info.java
I have converted this to use some of the now-current javadoc convetions.
src/java.naming/share/classes/javax/naming/ldap/package-info.java line 200:
> 198: * if (respCtls != null) {
> 199: * // Find the one we want
> 200: * for (int i = 0; i < respCtls.length; i++) {
This is the only change that isn't a direct 1:1 conversion, I hope this ok.
-
PR Review: https://git.openjdk.org/jdk/pull/19529#pullrequestreview-2142622498
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1655320955
