Re: RFR: 8343752: The javadoc should contain a note about usages of requires transitive java.base; [v4]
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]
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]
> 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]
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]
> 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]
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]
> 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;
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;
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