eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
151 stars 120 forks source link

[23] JEP 467: Markdown Documentation Comments #2429

Open mpalat opened 2 months ago

mpalat commented 2 months ago

ref: https://openjdk.org/jeps/467

Summary Enable JavaDoc documentation comments to be written in Markdown rather than solely in a mixture of HTML and JavaDoc @-tags.

mpalat commented 1 month ago

Please check these -

  1. https://commonmark.org/ and https://github.com/commonmark/commonmark-spec/wiki/List-of-CommonMark-Implementations
akurtakov commented 1 month ago

There are both markdown and commonmark (for the subtle differences) implementations available in Eclipse Mylyn project (https://github.com/eclipse-mylyn/org.eclipse.mylyn/tree/main/mylyn.docs/wikitext/core) . This implementaiton is already used by LSP4E project so it might even be time to move a bundle or two to Eclipse Platform if it becomes requirement for both LSP4E and JDT. FYI @ruspl-afed

jarthana commented 1 month ago

Another point: If we were to use an existing bundle for implementation, we might have to move some of the code (for e.g. JavadocParser) from org.eclipse.jdt.core.compiler.batch to org.eclipse.jdt.core.

ruspl-afed commented 1 month ago

Thank you for referencing me @akurtakov

Regarding move of Wikitext from Eclipse Mylyn to Eclipse Platform: It does not look impossible, but it needs some third-party libraries that I would not welcome, like Guava.

stephan-herrmann commented 1 month ago

Another point: If we were to use an existing bundle for implementation, we might have to move some of the code (for e.g. JavadocParser) from org.eclipse.jdt.core.compiler.batch to org.eclipse.jdt.core.

I don't think any code in compiler.batch parses HTML, does it? It should be the same for markdown, then.

AFAICS all the interpretation of markup/down happens in jdt.ui, only. Am I missing anything?

iloveeclipse commented 1 month ago

AFAICS all the interpretation of markup/down happens in jdt.ui, only.

Nope. I recently had to fix org.eclipse.jdt.internal.core.JavadocContents and I believe there are few other places around that parse javadoc.

jarthana commented 1 month ago

Another point: If we were to use an existing bundle for implementation, we might have to move some of the code (for e.g. JavadocParser) from org.eclipse.jdt.core.compiler.batch to org.eclipse.jdt.core.

I don't think any code in compiler.batch parses HTML, does it? It should be the same for markdown, then.

AFAICS all the interpretation of markup/down happens in jdt.ui, only. Am I missing anything?

JavadocParser does quite a bit of work for this, for e.g. the Javadoc hover and view. There's some in jdt.ui, which are for partitions and such.

stephan-herrmann commented 1 month ago

So I stand corrected, jdt.core does a bit of HTML parsing.

But is it really done in the compiler?

iloveeclipse commented 1 month ago

I'm not sure when JavadocParser is called but looking at https://openjdk.org/jeps/467 it seems it is not only html to markdown is the problem but "classic" javadoc comment style /** */ is proposed to be replaced with /// - and that sounds like we need changes in batch compiler. Also links to symbols (types, methods, fields) are supposed to change to something like /// - [a field][String#CASE_INSENSITIVE_ORDER] which will most likely need some kind of compiler change.

stephan-herrmann commented 1 month ago

I'm not sure when JavadocParser is called but looking at https://openjdk.org/jeps/467 it seems it is not only html to markdown is the problem but "classic" javadoc comment style /** */ is proposed to be replaced with /// - and that sounds like we need changes in batch compiler. Also links to symbols (types, methods, fields) are supposed to change to something like /// - [a field][String#CASE_INSENSITIVE_ORDER] which will most likely need some kind of compiler change.

Correct, but still the compiler should not need to "understand" markdown. Also I assume interpreting the new link format will happen where HTML-anchors are handled now: in JavadocContents (i.e., model).

This would imply

In terms of architecture, my guess is still that only jdt.ui needs a new dependency.

stephan-herrmann commented 1 month ago

Also links to symbols (types, methods, fields) are supposed to change to something like /// - [a field][String#CASE_INSENSITIVE_ORDER] which will most likely need some kind of compiler change.

This, btw, is a bit tricky, because the link target may contain array types denoted with [], which requires some escaping to avoid confusion with the enclosing link syntax. This makes me think that hand coded parsing is most appropriate for this part.

stephan-herrmann commented 1 month ago

Regarding move of Wikitext from Eclipse Mylyn to Eclipse Platform: It does not look impossible, but it needs some third-party libraries that I would not welcome, like Guava.

@ruspl-afed thanks for mentioning Guava, which is notorious for causing version conflicts with, e.g., Xtext. Do you know if guava types are used in any API of wikitext? If so, then it will probably re-export guava and that would lock clients of wikitext to use that specific version of guava.

If not mentioned in any API, we might get away without any conflicts? (as multiple versions my coexist within equinox).

OTOH, how deeply is that dependency built into wikitext? I could imagine that since Java 8 its use is no longer super-relevant? If we'd lend a hand, could it perhaps be refactored to avoid guava?

stephan-herrmann commented 1 month ago

Since this issue affects several layers of our architecture, I'm afraid it could hit us badly when addressed too late.

Could people please weigh in whether the following break-down is appropriate:

  • o.e.j.c.compiler.batch needs to learn a new style of comments
  • for o.e.jdt.core we have two options:
    • manually parse the new link style, or
    • delegate to some markdown library for this task
  • full interpretation of markdown is only needed in o.e.jdt.ui.

In terms of architecture, my guess is still that only jdt.ui needs a new dependency.

ruspl-afed commented 1 month ago

@stephan-herrmann

Do you know if guava types are used in any API of wikitext?

I do not see them as a part of API signatures, however we have Guava usages in API types

If not mentioned in any API, we might get away without any conflicts? (as multiple versions my coexist within equinox).

Theoretically yes.

OTOH, how deeply is that dependency built into wikitext? I could imagine that since Java 8 its use is no longer super-relevant? If we'd lend a hand, could it perhaps be refactored to avoid guava?

Some types are relatively easy to replace, others may require some effort (special collections, string manipulations). In Mylyn we have a ticket to remove Guava, but I cannot promise any specific delivery date, since we only have a few part-time volunteers for the whole Mylyn codebase.

Moreover, I can see other dependencies involved:

and a bit custom way of obtaining language support service instance

iloveeclipse commented 1 month ago

In the past I've seen Guava breaking API very fast, I personally wouldn't like to have it in platform, even if multiple versions are possible, it would be not fun to deal with resulting problems if someone just wants "some" guava package which would available in different versions and of course all incompatible to each other.

jarthana commented 1 month ago

OK, so the general consensus seems to be doing the hard-parsing like we are doing in JavadocParser with the help of Scanner and let the jdt.ui use third party libraries for the UI part? I will create an issue for UI separately as that seems like the bigger ask.

stephan-herrmann commented 1 month ago

OK, so the general consensus seems to be doing the hard-parsing like we are doing in JavadocParser with the help of Scanner and let the jdt.ui use third party libraries for the UI part? I will create an issue for UI separately as that seems like the bigger ask.

See https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1472 :)

stephan-herrmann commented 12 hours ago

Comment moved to https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2738#issuecomment-2251314730