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-8298405: Support Markdown in the standard doclet #11701

Closed

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Dec 15, 2022

Support for Markdown comments in the standard doclet.

To enable Markdown in a comment, start the comment with /**md followed by whitespace. The syntax is as defined for CommonMark.

The work is in 3 parts:

  1. Update the Compiler Tree API to support Markdown tree nodes, containing strings of (uninterpreted) Markdown source code.
  2. Import commonmark-java into the jdk.javadoc module, to be able to convert Markdown strings to HTML.
  3. Update the standard doclet, to leverage the preceding two parts, to translate Markdown in documentation comments to Content nodes.

There are new tests both for the low level work in the Compiler Tree API, and for the overall high-level work in the doclet.

Background info: https://mail.openjdk.org/pipermail/javadoc-dev/2023-January/005563.html


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8298687 to be approved

Issues

  • JDK-8298405: Support Markdown in the standard doclet
  • JDK-8298687: Support Markdown in the standard doclet (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11701/head:pull/11701
$ git checkout pull/11701

Update a local copy of the PR:
$ git checkout pull/11701
$ git pull https://git.openjdk.org/jdk pull/11701/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11701

View PR using the GUI difftool:
$ git pr show -t 11701

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11701.diff

@jonathan-gibbons jonathan-gibbons marked this pull request as draft December 15, 2022 23:47
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 15, 2022

👋 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 Dec 15, 2022

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

  • 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 compiler compiler-dev@openjdk.org csr Pull request needs approved CSR before integration labels Dec 15, 2022
@jonathan-gibbons jonathan-gibbons marked this pull request as ready for review January 3, 2023 19:08
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 3, 2023
@mlbridge
Copy link

mlbridge bot commented Jan 3, 2023

Webrevs

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.

This is a welcome change and impressive work! My initial comments are inline.

@jonathan-gibbons
Copy link
Contributor Author

This is a welcome change and impressive work! My initial comments are inline.

Thanks; I think of it as the icing on the cake of the technical-debt cleanup we have been doing over the past few releases.

@pavelrappo
Copy link
Member

In case you've missed test failures in your GitHub Actions (GHA): test/langtools/tools/javac/lib/DPrinter.java. A trivial fix, really.

@AlanBateman
Copy link
Contributor

This seems a significant feature with several design choices that probably should be captured somewhere. Is is significant enough to have a JEP?

@jonathan-gibbons jonathan-gibbons changed the title JDK-8298405: Markdown support in the standard doclet JDK-8298405: Support Markdown in the standard doclet Jan 5, 2023
Add legal header to imported CommonMark source files
Always use Text nodes inside AttributeTree values
Unwrap <p> from "simple" paragraphs
@jonathan-gibbons
Copy link
Contributor Author

In case you've missed test failures in your GitHub Actions (GHA): test/langtools/tools/javac/lib/DPrinter.java. A trivial fix, really.

Fixed.

Share Markdown parser and renderer in instance of MarkdownHandler
public class Html5Entities {

private static final Map<String, String> NAMED_CHARACTER_REFERENCES = readEntities();
private static final String ENTITY_PATH = "/org/commonmark/internal/util/entities.txt";
Copy link
Member

@pavelrappo pavelrappo Jan 9, 2023

Choose a reason for hiding this comment

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

I see that you've added the missing entities.properties file, but renamed it to entities.txt. IIRC from our offline chat, it's due to how JDK build treats .properties files.

I wonder what would be better: (a) to keep the original extension, but amend the build to ignore this file, or (b) to do what you've done and possibly suggest a PR for CommonMark to do the same.

On the one hand, it's not a true Java .properties file as one might've though from its extension. On the other hand, this change of yours diverge us from the original snapshot of CommonMark.

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Jan 9, 2023

Choose a reason for hiding this comment

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

I like your option (b)

  • retain the change to use entities.txt
  • report the issue to CommonMark, if not actually suggest a PR.

FWIW, the rename is handled automatically by the process I use to import the source code. I think that automated fix-up is acceptable; manual fix-up that would be needed every time we update the source would not be acceptable.

@wimdeblauwe
Copy link

wimdeblauwe commented Jan 21, 2023

Is the work here done in a way that asciidoc ( https://docs.asciidoctor.org/ ) support is also possible?

@ExE-Boss
Copy link

There should probably be an option to make Markdown the default.

@pavelrappo
Copy link
Member

@wimdeblauwe, not sure if you've noticed it, but your comment was hidden: #11701 (comment). To make the comment visible and response possible, consider accepting TOU suggested by the bot. Thanks.

@wimdeblauwe
Copy link

@wimdeblauwe, not sure if you've noticed it, but your comment was hidden: #11701 (comment). To make the comment visible and response possible, consider accepting TOU suggested by the bot. Thanks.

Thanks for letting me know! Should be ok now.

@jonathan-gibbons
Copy link
Contributor Author

FYI, draft JEP: https://openjdk.org/jeps/8299906

@jonathan-gibbons
Copy link
Contributor Author

There should probably be an option to make Markdown the default.

That would lead to configuration complexity in any environment that had to support different kinds of comments in different libraries/modules/packages/files

@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented Feb 7, 2023

Is the work here done in a way that asciidoc ( https://docs.asciidoctor.org/ ) support is also possible?

Yes, and no. But mostly no.

Yes, in the sense that there are aspects of this work that might allow asciidoclet to be enhanced. And, the general methodology of this work is not specific to the details of Markdown.

No, in the sense that the proposal does not include the Standard Doclet supporting pluggable backends for different markup languages, and I don't think we want to go there, at least to begin with. I think it is more interesting to focus on getting broader support across the ecosystem for a limited set of markup languages than an open-ended set.

@jimisola
Copy link

jimisola commented Feb 8, 2023

Would also really like to see AsciiDoc support. Our development team use it for everything. Markdown is ok but AsciiDoc is much better for technical documentation.

What is necessary for AsciiDoc to be considered? Or rather, what is the process for it being taken into consideration?

@pavelrappo
Copy link
Member

@jimisola, not sure if you've noticed it, but your comment was hidden: #11701 (comment). To make the comment visible and response possible, consider accepting TOU suggested by the bot. Thanks.

Have you seen that notification from the bot? Regardless, the bot might need to use a better message/format for such communication.

@wimdeblauwe
Copy link

I had the same issue with not noticing the question from the bot. I think the main problem is that there is no email notification of that message. So unless you manually check the ticket, you don't notice it.

@magicus
Copy link
Member

magicus commented Feb 8, 2023

@wimdeblauwe Let's not turn this PR into a discussion about Skara (the Github bots of OpenJDK), but just a short explanation: There is no way we can get the email address of a random user on Github coming by and making a comment; users emails are generally not available through Github for privacy reasons. So we just can't do that.

However, we do tag the user, and they should get a Github notification for that. If they have "send email on notification" turned on, they will get an email as well.

But I think the message could perhaps be a bit more explicit about their original content being hidden. This is not immediately clear if you just read the start of the message. I'll open an enhancement request on Skara for that.

@jimisola
Copy link

jimisola commented Feb 21, 2023

Have you seen that notification from the bot? Regardless, the bot might need to use a better message/format for such communication.

No, thanks. I missed that. Accepted now.

Great that this got implemented. Would have been even better with asciidoc support 😉

@jonathan-gibbons
Copy link
Contributor Author

FYI, the PR has been withdrawn for now, pending additional design discussions.

@lfvjimisola
Copy link

lfvjimisola commented May 30, 2023

Hi @lfvjimisola, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user lfvjimisola for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@koppor
Copy link
Contributor

koppor commented Jan 2, 2024

Thank you so much for the work @jonathan-gibbons and @lahodaj (for the jshell part).

I can see the need for an universal solution allow to handle both Markdown and ASCIIDoc. However, offering Markdown would be a huge improvement compared to the existing solution (having to use HTML and IDE support took a long time to fix "trivial" things - e.g., proper HTML reformat https://youtrack.jetbrains.com/issue/IDEA-147601).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

10 participants