Codecov Report

❌ Patch coverage is 60.30769% with 129 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.368%. Comparing base (51d5210) to head (9ac0726).
⚠️ Report is 25 commits behind head on master.

@@               Coverage Diff               @@
##              master     #4875       +/-   ##
===============================================
- Coverage     58.405%   58.368%   -0.037%     
+ Complexity      2537      2534        -3     
===============================================
  Files            677       681        +4     
  Lines          39046     39302      +256     
  Branches        7089      7134       +45     
===============================================
+ Hits           22805     22940      +135     
- Misses         13341     13448      +107     
- Partials        2900      2914       +14     

I tried several markdown examples provided by the specification https://openjdk.org/jeps/467 without causing any parsing errors. Ultimately, why should we make changes in JP since it seems to work with the current version?

@jlerbsc I assumed you wanted this based on your comment in #4869. This PR treats markdown comments as a single comment instead of a series of line comments, which makes it easier for users to use them. With how line comments are handled, only the last line comment would be associated with the commented method. Any preceding comments would be orphans, so correctly handling those currently involves handling ranges manually.

In that sense, I still think the change here is beneficial, but if simply parsing the comments without errors or crashing is sufficient, then this PR isn't needed and can be closed without issue

@johannescoetzee I agree with you that markdown comments are multi-line comments similar to javadoc comments. What I don't understand is that grouping these elements into a common class or using a class hierarchy makes change more complex. Could you elaborate on this point?

@ftomassetti , what is your opinion on integrating Markdown comments into the existing comment architecture? Do you agree with the suggestion made in this PR? Personally, I don't know JP well enough to have a strong opinion on this subject, but it seems to me that this PR introduces additional complexity into comment management.

@jlerbsc I've thought about your question/concern some more and I've changed my mind. I don't think this is really a problem anymore.

I had the idea that grouping them into a common class while I was still using the first representation I tried for this (seen in the earlier commits). In that representation, the content field contained what is now returned by the getMarkdownContent method. In that version, the usual reconstruction of comments comment.getHeader() + comment.getContent() + comment.getFooter() no longer made sense, and in my opinion changing that anywhere javadoc comments were handled (including in user code) was not worth the benefit of uniting those comments.

In the current version proposed by this PR, this is no longer an issue. I would therefore propose doing the following:

1. Rename JavadocComment to TraditionalJavadocComment, following the naming convention from https://docs.oracle.com/javase/specs/jls/se25/html/jls-3.html#jls-3.7 (where it's called TraditionalComment, but keeping the Javadoc in the name is useful in JavaParser to distinguish it from BlockComment
2. Create an abstract JavadocComment class that is extended by both TraditionalJavadocComment and MarkdownComment
3. Update the Javadoc and JavadocParser classes to handle MarkdownComments, which should now be a fairly small change.

Please let me know if you agree with this approach. Once I hear back from you, I'll update the PR with what we've discussed.

I've pushed changes starting with the Rename JavadocComment to TraditionalJavadocComment commit that implement the shared ancestor for TraditionalJavadocComment and MarkdownComment that also demonstrate the code changes required to handle this case. Overall the changes on the user side will be fairly minor. The majority of the manual changes I had to make were for javaparser internal components. It's possible I missed something, but I've covered the things I found and I would, of course, fix any issues related to this that come up.

It looks like the test failures are slow tests that aren't run with mvn test by default. I'm updating expectations for those now

Edit: Done

@johannescoetzee Thank you for the valuable work you have done. For now, I will wait for @ftomassetti 's opinion to shed further light on how to proceed.

@jlerbsc Sounds good. Since this isn't required to parse Java 23 code, would you be open to accepting Java 25 contributions before this is finalised? I'm on vacation next week, but I've made good progress with handling module imports, so will likely have that ready soon after I'm back.

Hi @johannescoetzee , I am going to ask you to modify your PR to simplify it and isolate the integration of Markdown comments from the current comment management. Would it be possible for you to separate the comment class hierarchy into a separate PR (this would involve preparing the code to support Markdown comments) and then create a new PR to manage Markdown comments?

Do you think that a solution consisting, initially, of parsing Markdown comments as inline comments and then creating and transforming these comments into Markdown in post-processing would be easier to maintain than a solution consisting of parsing Markdown comments directly?

Do you think that a solution consisting, initially, of parsing Markdown comments as inline comments and then creating and transforming these comments into Markdown in post-processing would be easier to maintain than a solution consisting of parsing Markdown comments directly?

I did consider this approach as well, but thought it would end up being more complex. The main problem is that all but the last line comment before a method end up as orphan comments, so, to correctly reconstruct the markdown comments, it would necessary to connect these line comments by looking at token ranges and the tokens between them. While separating this logic from the parser would bring some benefit, I think it would be harder to reason about than the approach implemented in this PR where the token stream is being processed directly. It would also only work if tokens are saved (as far as I understand), which is a significant downside.

Do you think that a solution consisting, initially, of parsing Markdown comments as inline comments and then creating and transforming these comments into Markdown in post-processing would be easier to maintain than a solution consisting of parsing Markdown comments directly?

I did consider this approach as well, but thought it would end up being more complex. The main problem is that all but the last line comment before a method end up as orphan comments, so, to correctly reconstruct the markdown comments, it would necessary to connect these line comments by looking at token ranges and the tokens between them. While separating this logic from the parser would bring some benefit, I think it would be harder to reason about than the approach implemented in this PR where the token stream is being processed directly. It would also only work if tokens are saved (as far as I understand), which is a significant downside.

Could you add this analysis, along with the other approaches you considered, to the description of the PR, in order to document in the JP project the approach that was chosen and those that were considered? Thank you.

Can you close this PR if it is no longer relevant?

Closed in favour of #4885 and #4899