tnorbye / kdoc-formatter

Reformats Kotlin KDoc comments, reflowing text and other cleanup, both via IDE plugin and command line utility
Apache License 2.0
73 stars 2 forks source link

Preformatting stability #78

Closed davidtorosyan closed 2 years ago

davidtorosyan commented 2 years ago

Stability bug with <pre>.

Before:

/**
 * Some summary
 *
 * <pre>
 * line one
 * ```
 *     line two
 * ```
 */
fun foo() = Unit

After:

/**
 * Some summary
 *
 * ```
 *
 * line one
 *
 * ```
 *     line two
 * ```
 */
fun foo() = Unit

After again:

/**
 * Some summary
 *
 * ```
 *
 * line one
 *
 * ```
 *
 * line two
 *
 * ```
 */
fun foo() = Unit

Using v1.5.5 with default options

tnorbye commented 2 years ago

The problem here is that this isn't valid markup -- the opening <pre> tag isn't closed. After recent bug fixes I tried to gracefully handle this; it isn't entirely clear what should happen, but dokka does for example interpret later @param tags as parameter documentation, so what I settled on was to "ignore" the <pre> tag.

However, the <pre> tag was still getting converted to ```. And what happens here is that it then after formatting gets paired up with the first ``` in the initial comment.

I'm fixing this such that an unmatched <pre> is not converted to ```. With that, the formatting is stable.

(An alternative would have been for an unmatched <pre> to simply apply to the rest of the comment, so nothing after <pre> gets adjusted. I'll play with this and see if it feels better. Do you have a lot of cases where people leave <pre> comments unterminated? (The Dokka formatting for that does not look good.)

tnorbye commented 2 years ago

I played with it a bit and I think I'll switch it such that if you have a <pre> tag, without a closing </pre> tag, instead of just treating that <pre> line as preformatted, the entire remainder of the comment is treated as preformatted.

tnorbye commented 2 years ago

(I tried inserting a final </pre> on the last line, but when I do that, a subsequent format will then turn this into ```-blocks so the formatter isn't stable. I then tried to have it directly convert the <pre> into ``` at the same time, but in a case like the above, where you already have a mixture of ``` and <pre> this made the markup even more confused. I think trying to interpret broken markup is a losing battle, which is why just treating an unterminated <pre> and leaving things alone seems like the safest course.)

davidtorosyan commented 2 years ago

This isn't really a missing </pre> bug, it repros for this as well:

/**
 * Some summary
 *
 * <pre>
 * ```
 *     code
 * ```
 * </pre>
 */
fun foo() = Unit

I'm actually fine with the final output - maybe it could be better in some cases, but I agree it's a losing battle.

What I'm more concerned with is having the formatter be idempotent. Could this be solved by doing a second pass? Or maybe by doing replacements before reflowing?

tnorbye commented 2 years ago

Both of these should be handled in 1.5.7. It will treat an unterminated <pre> such that the remainder of the comment is treated as preformatted -- and when it is terminated, but doubled up with ``, it will not convert the redundant

` tags.