-
Notifications
You must be signed in to change notification settings - Fork 163
[Formatter] Recognise MarkDown headings, Lists, Comment Snippets & Tables in formatter #4418
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
Conversation
|
Hi @mateusz-matela, please check this once you are available |
|
I don't think a setting for this is necessary. It made more sense for html tags, as one can put spaces and newlines inside/around them in many different ways that make sense, so one may not want to use the formatter's standard. But for markdown elements there's pretty much only one sensible notation, wouldn't you agree? Note that markdown also supports tables which require a similar approach: https://docs.oracle.com/en/java/javase/23/javadoc/using-markdown-documentation-comments.html#GUID-7269D6B1-BAA2-4260-A295-EBC3DDA3E69C After a glance at the code I can say that the new code would better fit in a separate |
Then I guess I'll remove the UI and option changes 👍 |
maybe this point is actually even stronger - if the setting is turned off, the formatter would mangle the headers and lists into one line, resulting in completely different javadoc content? Nobody would want that! As for tables - I guess they are much more complicated due to columns alignment - maybe that could be a separate issue (with an on/off setting this time?), but for now we'd need to at least detect them to make sure they are not touched. |
Yes, and different types of comments too https://docs.oracle.com/en/java/javase/23/javadoc/using-markdown-documentation-comments.html#GUID-7E6D6A81-1176-4CF2-85EE-97A86ACDA351 this I will do in another PR. I had hard time implementing this |
Agreed :D
Lists where complicated too due to indent style, but was manageable. For table pattern recognition do you have any suggestions ? |
f92dfce to
bf1e64b
Compare
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Fixed
Show fixed
Hide fixed
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Fixed
Show fixed
Hide fixed
ce5d986 to
b628847
Compare
b628847 to
eb48ce3
Compare
84dda38 to
86b0e4f
Compare
|
Hi @mateusz-matela SnippetsSnippetMarkdown.mp4Tablesfor tables currently the formatter recognise the pattern, however it still have some issues in processing column arrangement but it wont break the table structure now. TableMarkdown.mp4Could you please check the new changes ? |
|
Unrelated test failure |
mateusz-matela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for tables currently the formatter recognise the pattern, however it still have some issues in processing column arrangement but it wont break the table structure now.
I think full support would mean parsing the whole table, reformatting contents of each cell and determining width of each column as max width of column's cells in order to align everything. This would require a setting as someone my want to keep their unconventional alignment for some reason. I'm not sure it makes sense to implement a partial solution for now, it would be enough to leave tables untouched (as if the new setting is off)
....jdt.core.tests.model/src/org/eclipse/jdt/core/tests/formatter/FormatterRegressionTests.java
Outdated
Show resolved
Hide resolved
....jdt.core.tests.model/src/org/eclipse/jdt/core/tests/formatter/FormatterRegressionTests.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/TokenManager.java
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/TextEditsBuilder.java
Outdated
Show resolved
Hide resolved
....jdt.core.tests.model/src/org/eclipse/jdt/core/tests/formatter/FormatterRegressionTests.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
085f2f3 to
76a755f
Compare
a7f4eff to
b277e18
Compare
Oh right, good new tests ;) Fixed with mateusz-matela@e1815a5 |
b277e18 to
195c200
Compare
I tried out the new changes but yet its failing with 195c200 :( |
Hmm, I just cherry picked my last two commits on top of yours and all tests pass :) |
195c200 to
7616659
Compare
7616659 to
b7b2e0c
Compare
|
@mateusz-matela I have added both of the commits now 👍 |
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
| int endPos = -1; | ||
| int tokenIndexLast = -1; | ||
| Token closingToken = null; | ||
| boolean shouldDisable = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found in CommonMark spec:
If the end of the containing block (or document) is reached and no closing code fence has been found, the code block contains all of the lines after the opening code fence until the end
So maybe it was good to init with shouldDisable = true but also closing Token should init as the last token in the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check this.. it looks like a very tricky edge case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled this case now 👍
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
| String fenceCharsEnd = matcher.group(1); // the fence itself | ||
| int fenceLengthEnd = fenceCharsStart.length(); | ||
| // Check if fences match | ||
| if (!fenceCharsStart.equals(fenceCharsEnd)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also check if there's no equivalent of info string after the closing fence, turns out it is not allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, it's also not allowed to have more than 3 spaces in front of a fence, not counting any indentation coming from lists. So code blocks analysis would need to be part of handleMarkdownList... So maybe that's for later :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will check this later then..
org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter/CommentsPreparator.java
Outdated
Show resolved
Hide resolved
25ea122 to
baae5f9
Compare
|
@mateusz-matela If there are no more major issues shall we proceed with current state ? |
|
@SougandhS : not sure why this was assigned to me, I've undit that. I have no insights into the "formatter" domain area. |
|
The fenced blocks implementation become quite complex, so I thought I'd have a try at the target solution and integrate it with the lists implementation: mateusz-matela@e344bd9 |
8afb6e8 to
3a0e8b0
Compare
Thanks, I have added the changes |
This commits adds the support to recognize markdown Headings, Lists, Fences & Tables for formatter Fixes : eclipse-jdt#4337
3a0e8b0 to
582da6b
Compare
|
Thanks a lot for the thorough review @mateusz-matela ! |
This commits adds the support to recognize markdown headings, lists, comment fences & tables for formatter
fixes :
#4337
Lists
Lists.mp4
Headings
Headings.mp4
Snippets
SnippetMarkdown.mp4
What it does
How to test
Author checklist