Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec) #790

Closed
wants to merge 12 commits into from

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Oct 22, 2020

This introduces support for a new @spec tag that can be used as either an inline tag or as a block tag. It is used to identify references to external specifications, in such a way that the references can be collected together on a new summary page, called "Other Specifications", available from either the static INDEX page or the interactive search box.

As an inline tag, the format is {@spec url label}, which is roughly translated to <a href="url">label</a> in the generated docs.

As a block tag, the format is simply

@spec url label

which is handled in a manner analogous to

@see <a href="url">label</a>

The tag is notable for being the first standard/supported tag that can be used as either an inline or block tag. (We have had support for bimodal non-standard/custom tags for a while, such as {@jls} and {@jvms}.) To support bimodal standard tags, some changes to DocCommentParser are incorporated here.

This change is only the support for the new tag; it does not include any changes to API docs to use the new tag.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (6/6 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issues

  • JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
  • JDK-8226279: javadoc should support a new @SPEC tag

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/790/head:pull/790
$ git checkout pull/790

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 22, 2020

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Oct 22, 2020

@jonathan-gibbons The following labels will be automatically applied to this pull request:

  • build
  • compiler
  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added javadoc javadoc-dev@openjdk.org build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Oct 22, 2020
@jonathan-gibbons
Copy link
Contributor Author

/issue add JDK-8226279

@openjdk
Copy link

openjdk bot commented Oct 23, 2020

@jonathan-gibbons
Adding additional issue to issue list: 8226279: javadoc should support a new @spec tag.

@jonathan-gibbons
Copy link
Contributor Author

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Oct 23, 2020
@openjdk
Copy link

openjdk bot commented Oct 23, 2020

@jonathan-gibbons has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@jonathan-gibbons please create a CSR request and add link to it in JDK-6251738. This pull request cannot be integrated until the CSR request is approved.

@jonathan-gibbons jonathan-gibbons marked this pull request as ready for review October 23, 2020 15:46
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 23, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 23, 2020

Webrevs

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes look good.

Copy link
Member

@hns hns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very instructive to see a new tag introduced from scratch with all the accompanying infrastructure and tests. The changes looks good to me, but I need to stress than I'm not an expert on some of the affected areas such as the jdk.compiler parts.

@openjdk
Copy link

openjdk bot commented Nov 4, 2020

@jonathan-gibbons this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout new-spec-tag
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 4, 2020
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 4, 2020
/**
* Creates a new {@code SpecTree} object, to represent a {@code @spec} tag.
*
* @param inline whether this is instance is an inline tag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be:
* @param inline whether this instance is an inline tag

Copy link
Member

@pavelrappo pavelrappo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Thanks for incorporating my previous offline feedback.
  2. Since Hannes and Erik seem to have looked at everything else, I looked mostly at changes in src/jdk.compiler/share/classes/com/sun/source/**, which are good!
  3. There should be a corresponding but separate change to "Documentation Comment Specification for the Standard Doclet".
  4. Can we use this new @since tag to refer to the spec at com/sun/tools/javac/parser/DocCommentParser.java:1116?
  5. Should we specify in com.sun.source.doctree.SpecTree that both url and label parts are mandatory?
  6. DCSpec extends DCEndPosTree, sigh... Although that is not a public API, this design suggests we could improve that abstraction sometime later.

}

DCTree.Kind getTreeKind() {
return treeKind;
}

abstract DCTree parse(int pos) throws ParseException;
DCTree parse(int pos, Kind kind) throws ParseException {
if (kind != this.kind && this.kind != Kind.EITHER) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition looks right, but shouldn't we add another one to guard against accidental passing of kind == EITHER?

@@ -300,6 +301,7 @@ public static String encodeURL(String url) {

/**
* Creates an HTML {@code A} element.
* The {@code ref} argument will be URL-encoded for use as the attribute value.
*
* @param ref the value for the {@code href} attribute}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are here, could you please remove that trailing }?

* and will <i>not</i> be additionally URL-encoded, but will be
* {@link URI#toASCIIString() converted} to ASCII for use as the attribute value.
*
* @param ref the value for the {@code href} attribute}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more, perhaps copy-pasted, trailing }.

@@ -0,0 +1,32 @@
/*
* Copyright (c) 2012, 2020 Oracle and/or its affiliates. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be:
Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights reserved.

@@ -257,6 +263,13 @@ public Content seeTagOutput(Element holder, List<? extends SeeTree> seeTags) {
HtmlTree.DD(body));
}

String textOf(List<? extends DocTree> trees) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this method should work recursively rather than shallowly. Consider the following example:

@spec http://example.com Some code: {@code text}

I reckon we'll end up with only Some code: .

Comment on lines +74 to +81
public boolean isBlank() {
return false;
}

public static boolean isBlank(List<? extends DCTree> list) {
return list.stream().allMatch(DCTree::isBlank);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these two methods here might be overkill.

@@ -733,7 +734,7 @@ private void showTaglets(PrintStream out) {
taglets.addAll(allTaglets.values());

for (Taglet t : taglets) {
String name = t.isInlineTag() ? "{@" + t.getName() + "}" : "@" + t.getName();
String name = t.isBlockTag() ? "@" + t.getName() : "{@" + t.getName() + "}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why did you flip that?

@jonathan-gibbons
Copy link
Contributor Author

Closing pull request, until we better decide the contents of the spec page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org compiler compiler-dev@openjdk.org csr Pull request needs approved CSR before integration javadoc javadoc-dev@openjdk.org rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

4 participants