On Thu, 9 Sep 2021 15:27:34 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> This PR implements JEP 413 "Code Snippets in Java API Documentation", which >> hasn't been yet proposed to target JDK 18. The PR starts as a squashed merge >> of the https://github.com/openjdk/jdk-sandbox/tree/jdk.javadoc/snippets >> branch. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Remove trailing whitespace to satisfy jcheck The primary actionable item is that all strings that are intended for use in end-user error messages need to be localizable. i.e. see `Parser.ParseException` Other comments are minor and can be deferred. src/jdk.compiler/share/classes/com/sun/source/util/SimpleDocTreeVisitor.java line 460: > 458: * @return the result of {@code defaultAction} > 459: * @since 18 > 460: */ optional: reformat the latter part of the comment to align param descriptions ... i.e. use IntelliJ IDEA reformat code src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java line 1077: > 1075: return ch == ' ' || ch == '\t' /* || ch == '\f'*/; > 1076: } > 1077: It would be good to resolve this issue sooner rather than later, but not right now. I suggest we file a JBS issue to make a determination and update the doc comment world accordingly. Yes, it will be a minor change for DocCommentParser. The JBS issue will provide a good place to record the decision and its rationale. src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java line 1447: > 1445: throw new ParseException("dc.no.content"); > 1446: } else { > 1447: throw new > ParseException("dc.unexpected.content"); Heads up, there may be a merge conflict with the upcoming work on `DCTree` diagnostic positions. `ParseException` will soon take an optional "pos" parameter to better specify the position at which the error was detected. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java line 404: > 402: e = getLinkedElement(element, t); > 403: if (e == null) { > 404: // diagnostic output Use TODO? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java line 444: > 442: * the provided signature; returns null if such element could not be > found. > 443: * > 444: * This method is to be used when it's the target of the link that is very minor ... this would read better without the contraction (replace "it's" with "it is") src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java line 455: > 453: var fabricatedPath = new DocTreePath(rootPath, reference); > 454: return utils.docTrees.getElement(fabricatedPath); > 455: } This feels like a plausible addition to the `DocTrees`API, where it ought to be possible to do it without creating the `ReferenceTree` and `DoctreePath` wrapper objects. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 200: > 198: var path = > writer.configuration().utils.getCommentHelper(holder) > 199: .getDocTreePath(snippetTag.getBody()); > 200: // FIXME: there should be a method in Messages; that method > should mirror Reporter's; use that method instead accessing Reporter. downgrade to TODO? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 270: > 268: * diff view, but for now simply outputting both sides of a hybrid > snippet > 269: * would do. A user could then use a diff tool of their choice to > compare > 270: * those sides. For discussion ... a partial solution that may be good enough is to identify the position (e.g. line+offset) of the first character that is different. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/MarkupParser.java line 87: > 85: if (!Character.isUnicodeIdentifierStart(ch)) { > 86: // TODO: internationalize! > 87: throw new ParseException("Bad character: '%s' > (0x%s)".formatted(ch, Integer.toString(ch, 16)), bp); Heads up: this should be updated when `ParseException` takes a position. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/ParseException.java line 50: > 48: * @param position the approximate position > 49: */ > 50: public ParseException(String message, int position) { I know it is common to use English-only strings in exceptions that are not intended to be seen in normal use by an end user, but here, you're using `ParseException` to contain content that is intended to appear in end-user error messages, triggered by errors in the content provided by an end-user. As such, it is close to a firm requirement that these messages can be localized. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 85: > 83: // Incomplete actions waiting for their complementary @end > 84: private final Regions regions = new Regions(); > 85: // List of tags; consumed from top to bottom "top to bottom" is somewhere between ambiguous and confusing. Is it file-centric or queue-as-stack -centric? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 217: > 215: case "link" -> { > 216: var target = attributes.get("target", > Attribute.Valued.class) > 217: .orElseThrow(() -> new > ParseException("target is absent", internationalize src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 234: > 232: case "replace" -> { > 233: var replacement = attributes.get("replacement", > Attribute.Valued.class) > 234: .orElseThrow(() -> new > ParseException("replacement is absent", internationalize src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 255: > 253: case "start" -> { > 254: var region = attributes.get("region", > Attribute.Valued.class) > 255: .orElseThrow(() -> new > ParseException("Unnamed start", internationalize ------------- PR: https://git.openjdk.java.net/jdk/pull/4795
