Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v4]

2024-11-12 Thread Hannes Wallnöfer
On Mon, 11 Nov 2024 07:25:38 GMT, Jan Lahoda  wrote:

>> When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be 
>> a new preview language feature: `requires transitive java.base;`. It would 
>> be good to show a warning box in the javadoc about the use of the feature. 
>> This PR is attempting to add such a warning box.
>> 
>> Given it is likely this feature will either graduate or be dropped in the 
>> span of a few releases, I tried to make the code (relatively) easy to 
>> remove: it should be enough to delete the `if (javaBase != null && 
>> indirectPackages.keySet().contains(javaBase)) {` and the then section, 
>> keeping the else section + some slight cleanup.
>> 
>> The new javadoc can be seen here:
>> https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removing divs, as suggested

Looks good to me.

-

Marked as reviewed by hannesw (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21949#pullrequestreview-2429108522


Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v3]

2024-11-11 Thread Jan Lahoda
On Fri, 8 Nov 2024 17:32:08 GMT, Hannes Wallnöfer  wrote:

>> Jan Lahoda has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Using  instead of , as suggested.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ModuleWriter.java
>  line 605:
> 
>> 603: //add the preview box:
>> 604: section.add(HtmlTree.DIV(HtmlTree.BR()));
>> 605: section.add(HtmlTree.DIV(HtmlTree.BR()));
> 
> I just saw this in the generated HTML. Using `` tags to add vertical 
> space is ok, but there's no need to put them inside a `` element. Adding 
> only the `` tags should have the same effect.

Fixed in: 
https://github.com/openjdk/jdk/pull/21949/commits/0bb785132f0c8916c4ec7c8c69c56715d5b5a973.
 Thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21949#discussion_r1836104008


Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v4]

2024-11-10 Thread Jan Lahoda
> When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be a 
> new preview language feature: `requires transitive java.base;`. It would be 
> good to show a warning box in the javadoc about the use of the feature. This 
> PR is attempting to add such a warning box.
> 
> Given it is likely this feature will either graduate or be dropped in the 
> span of a few releases, I tried to make the code (relatively) easy to remove: 
> it should be enough to delete the `if (javaBase != null && 
> indirectPackages.keySet().contains(javaBase)) {` and the then section, 
> keeping the else section + some slight cleanup.
> 
> The new javadoc can be seen here:
> https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base

Jan Lahoda has updated the pull request incrementally with one additional 
commit since the last revision:

  Removing divs, as suggested

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/21949/files
  - new: https://git.openjdk.org/jdk/pull/21949/files/22cb35b8..0bb78513

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=21949&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21949&range=02-03

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/21949.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/21949/head:pull/21949

PR: https://git.openjdk.org/jdk/pull/21949


Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v3]

2024-11-08 Thread Hannes Wallnöfer
On Fri, 8 Nov 2024 16:58:52 GMT, Jan Lahoda  wrote:

>> When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be 
>> a new preview language feature: `requires transitive java.base;`. It would 
>> be good to show a warning box in the javadoc about the use of the feature. 
>> This PR is attempting to add such a warning box.
>> 
>> Given it is likely this feature will either graduate or be dropped in the 
>> span of a few releases, I tried to make the code (relatively) easy to 
>> remove: it should be enough to delete the `if (javaBase != null && 
>> indirectPackages.keySet().contains(javaBase)) {` and the then section, 
>> keeping the else section + some slight cleanup.
>> 
>> The new javadoc can be seen here:
>> https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Using  instead of , as suggested.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ModuleWriter.java
 line 605:

> 603: //add the preview box:
> 604: section.add(HtmlTree.DIV(HtmlTree.BR()));
> 605: section.add(HtmlTree.DIV(HtmlTree.BR()));

I just saw this in the generated HTML. Using `` tags to add vertical space 
is ok, but there's no need to put them inside a `` element. Adding only 
the `` tags should have the same effect.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21949#discussion_r1834811279


Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v3]

2024-11-08 Thread Jan Lahoda
> When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be a 
> new preview language feature: `requires transitive java.base;`. It would be 
> good to show a warning box in the javadoc about the use of the feature. This 
> PR is attempting to add such a warning box.
> 
> Given it is likely this feature will either graduate or be dropped in the 
> span of a few releases, I tried to make the code (relatively) easy to remove: 
> it should be enough to delete the `if (javaBase != null && 
> indirectPackages.keySet().contains(javaBase)) {` and the then section, 
> keeping the else section + some slight cleanup.
> 
> The new javadoc can be seen here:
> https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base

Jan Lahoda has updated the pull request incrementally with one additional 
commit since the last revision:

  Using  instead of , as suggested.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/21949/files
  - new: https://git.openjdk.org/jdk/pull/21949/files/8b8d8296..22cb35b8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=21949&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21949&range=01-02

  Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/21949.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/21949/head:pull/21949

PR: https://git.openjdk.org/jdk/pull/21949


Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v2]

2024-11-08 Thread Hannes Wallnöfer
On Fri, 8 Nov 2024 10:17:04 GMT, Jan Lahoda  wrote:

>> When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be 
>> a new preview language feature: `requires transitive java.base;`. It would 
>> be good to show a warning box in the javadoc about the use of the feature. 
>> This PR is attempting to add such a warning box.
>> 
>> Given it is likely this feature will either graduate or be dropped in the 
>> span of a few releases, I tried to make the code (relatively) easy to 
>> remove: it should be enough to delete the `if (javaBase != null && 
>> indirectPackages.keySet().contains(javaBase)) {` and the then section, 
>> keeping the else section + some slight cleanup.
>> 
>> The new javadoc can be seen here:
>> https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base
>
> Jan Lahoda 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 seven additional commits since 
> the last revision:
> 
>  - Adding forgotten file.
>  - Merge branch 'JDK-8335989' into 
> preview-javadoc-requires-transitive-java.base
>  - Better detection of requires transitive java.base; - fixes tests.
>  - Cleanup: avoid unnecessary bundle key.
>  - Adding a test, adjusting the message.
>  - Improving the UI, as suggested.
>  - Creating a separate javadoc section for the preview 'requires transitive 
> java.base;' in the java.se module.

Changes look good, it's nice that you kept the code changes in a single blob.

>From an HTML point of view, if the purpose of the `` elements in the 
>preview message is just to cause a line break, it would be slightly simpler 
>and more idiomatic to just append a `` tag:

line1\
line2\
...

-

Marked as reviewed by hannesw (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21949#pullrequestreview-2424125959


Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v2]

2024-11-08 Thread Jan Lahoda
> When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be a 
> new preview language feature: `requires transitive java.base;`. It would be 
> good to show a warning box in the javadoc about the use of the feature. This 
> PR is attempting to add such a warning box.
> 
> Given it is likely this feature will either graduate or be dropped in the 
> span of a few releases, I tried to make the code (relatively) easy to remove: 
> it should be enough to delete the `if (javaBase != null && 
> indirectPackages.keySet().contains(javaBase)) {` and the then section, 
> keeping the else section + some slight cleanup.
> 
> The new javadoc can be seen here:
> https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base

Jan Lahoda 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 seven additional commits since 
the last revision:

 - Adding forgotten file.
 - Merge branch 'JDK-8335989' into preview-javadoc-requires-transitive-java.base
 - Better detection of requires transitive java.base; - fixes tests.
 - Cleanup: avoid unnecessary bundle key.
 - Adding a test, adjusting the message.
 - Improving the UI, as suggested.
 - Creating a separate javadoc section for the preview 'requires transitive 
java.base;' in the java.se module.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/21949/files
  - new: https://git.openjdk.org/jdk/pull/21949/files/0bebf867..8b8d8296

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=21949&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21949&range=00-01

  Stats: 299476 lines in 1988 files changed: 125495 ins; 150738 del; 23243 mod
  Patch: https://git.openjdk.org/jdk/pull/21949.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/21949/head:pull/21949

PR: https://git.openjdk.org/jdk/pull/21949


Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base;

2024-11-07 Thread Adam Sotona
On Thu, 7 Nov 2024 12:03:18 GMT, Jan Lahoda  wrote:

> When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be a 
> new preview language feature: `requires transitive java.base;`. It would be 
> good to show a warning box in the javadoc about the use of the feature. This 
> PR is attempting to add such a warning box.
> 
> Given it is likely this feature will either graduate or be dropped in the 
> span of a few releases, I tried to make the code (relatively) easy to remove: 
> it should be enough to delete the `if (javaBase != null && 
> indirectPackages.keySet().contains(javaBase)) {` and the then section, 
> keeping the else section + some slight cleanup.
> 
> The new javadoc can be seen here:
> https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base

Looks good to me.

-

Marked as reviewed by asotona (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21949#pullrequestreview-2421073380


RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base;

2024-11-07 Thread Jan Lahoda
When https://github.com/openjdk/jdk/pull/21431 is integrated, there will be a 
new preview language feature: `requires transitive java.base;`. It would be 
good to show a warning box in the javadoc about the use of the feature. This PR 
is attempting to add such a warning box.

Given it is likely this feature will either graduate or be dropped in the span 
of a few releases, I tried to make the code (relatively) easy to remove: it 
should be enough to delete the `if (javaBase != null && 
indirectPackages.keySet().contains(javaBase)) {` and the then section, keeping 
the else section + some slight cleanup.

The new javadoc can be seen here:
https://cr.openjdk.org/~jlahoda/8343752/updated/api/java.se/module-summary.html#preview-requires-transitive-java.base

-

Depends on: https://git.openjdk.org/jdk/pull/21431

Commit messages:
 - Cleanup: avoid unnecessary bundle key.
 - Adding a test, adjusting the message.
 - Improving the UI, as suggested.
 - Creating a separate javadoc section for the preview 'requires transitive 
java.base;' in the java.se module.

Changes: https://git.openjdk.org/jdk/pull/21949/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21949&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8343752
  Stats: 71 lines in 3 files changed: 66 ins; 1 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/21949.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/21949/head:pull/21949

PR: https://git.openjdk.org/jdk/pull/21949