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

[23] JEP 467: Render markdown doc comments #1523

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

stephan-herrmann
Copy link
Contributor

@stephan-herrmann stephan-herrmann commented Jul 21, 2024

  • add dependency org.commonmark incl. extension for tables
  • use it for rendering
    • markdown variant is detected by inspecting the raw javadoc text, might be replaced by new API in DOM
  • minimal tests incl. examples from the specification in https://openjdk.org/jeps/467

fixes #1472

+ dependency org.commonmark & strawman usage thereof
@stephan-herrmann stephan-herrmann added this to the BETA_JAVA23 milestone Jul 21, 2024
@stephan-herrmann stephan-herrmann self-assigned this Jul 21, 2024
@stephan-herrmann
Copy link
Contributor Author

@merks This PR boldly requires org.commonmark (actual client is org.eclipse.jdt.core.manipulation), and expectedly fails with

12:00:54  [ERROR] Cannot resolve project dependencies:
12:00:54  [ERROR]   Software being installed: org.eclipse.jdt.core.manipulation 1.21.200.qualifier
12:00:54  [ERROR]   Missing requirement: org.eclipse.jdt.core.manipulation 1.21.200.qualifier requires 'osgi.bundle; org.commonmark 0.22.0' but it could not be found

How do you suggest to fix this?

  • Can you add org.commonmark to eclipse-sdk-prereqs.target?
    • Apparently, that repo doesn't have a BETA_JAVA23 branch, so would it be OK, to add the dep in master?
  • Should I override the configuration for tycho's target-platform-configuration ?
    • org.eclipse.jdt.core.manipulation is pom-less, so I guess the change would go into eclipse.jdt.ui root pom.

@merks
Copy link
Contributor

merks commented Jul 21, 2024

Please add it to the target in https://github.com/eclipse-platform/eclipse.platform.releng.aggregator

I’m away from the office but can review a PR if you add the to the orbit location in the target.

@stephan-herrmann stephan-herrmann marked this pull request as ready for review August 11, 2024 16:05
@stephan-herrmann
Copy link
Contributor Author

This PR still shows the 4 failures from #1526 which are fixed in master by eclipse-jdt/eclipse.jdt.core#2747 so JDT/Core needs its beta branch updated.

@jarthana @mpalat are you doing merges master->BETA_JAVA23 by some schedule or just as needed?

@jarthana
Copy link
Member

This PR still shows the 4 failures from #1526 which are fixed in master by eclipse-jdt/eclipse.jdt.core#2747 so JDT/Core needs its beta branch updated.

@jarthana @mpalat are you doing merges master->BETA_JAVA23 by some schedule or just as needed?

No fixed scheduled per se. Will do it today.

@jjohnstn jjohnstn merged commit d2fc5ce into eclipse-jdt:BETA_JAVA23 Aug 13, 2024
3 of 6 checks passed
@jjohnstn
Copy link
Contributor

Hi @stephan-herrmann I seemed to have caused a git accident. I was trying to merge all the PRs for markdown in my local branch using gh pr merge and it actually merged it here. I will have to revert. My apologies. I do think the patch is valid but I couldn't test it without the partitioning logic.

@jjohnstn
Copy link
Contributor

Ok, I reverted the merged changes and then created a revert of the revert to add back the original PR commits. See #1586

@stephan-herrmann
Copy link
Contributor Author

OMG, bye bye clean history ;-P

FYI, I have one more commit in my local branch, but all this waits for necessary changes in and around eclipse-jdt/eclipse.jdt.core#2807.

Locally, all markdown hovers look nice already :)

I might as well start a fresh new PR, once the core part is ready.

As we are doing incremental work in the face of a deadline approaching, perhaps you could leave the merging in BETA_JAVA23 to me, while I coordinate with Jay?

@stephan-herrmann
Copy link
Contributor Author

BTW, @jjohnstn when I edit the new class CoreMarkdownAccessImpl saving consistently removes a required import. Do you observe the same? My build is 20240813-0159

@stephan-herrmann stephan-herrmann deleted the issue1472_markdown branch August 13, 2024 23:05
@jjohnstn
Copy link
Contributor

@stephan-herrmann I had no intention of merging anything in BETA_JAVA23. There was a stack overflow posting that mentioned how to get multiple PR patches using merge and I tried using gh which was the wrong thing to do. I'll take a look at the removed import.

@jjohnstn
Copy link
Contributor

@stephan-herrmann Did you mean 20240813-0100 from https://download.eclipse.org/eclipse/downloads/ ? I tried the Linux x86_64 version to edit the BETAJAVA_23 checked out branches of jdt.ui, jdt.core, and jdt.debug. The project settings for jdt.core.manipulation has organize imports on save, but I see no changes after some minor edits and it properly adds any new collection classes I refer to. In my Target Platform I point to 4.33-I-builds and https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/milestone/S202407231258/ for the commonmark plug-ins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants