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

consider queued newlines when wrapping elements in MarkupFormatter #116

Merged
merged 5 commits into from Jul 20, 2023

Conversation

QuietMisdreavus
Copy link
Contributor

Bug/issue #, if applicable: rdar://106915293

Summary

When an inline code span, image, link, or inline attribute is written directly after a soft or hard line break, the MarkupFormatter incorrectly adds an extra line break before the latter item. This leads to Markdown in these situations being incorrectly round-tripped, because now there is a paragraph break that wasn't there before. This PR makes the MarkupFormatter consider queued newlines in these checks.

This also fixes an error in the MarkupFormatter output for inline attributes, which were previously missing the caret that denoted them as inline attributes instead of links.

Dependencies

None

Testing

Use the following markdown file:

This is some text.
`This is some code.`

This is some text.
[This is a link.](https://swift.org)

This is some text.
![This is an image.](image.png)

This is some text.
^[This is some attributed text.](rainbow: 'extreme')

Steps:

  1. Save the previous Markdown as test.md.
  2. swift run markdown-tool format --check-structural-equivalence test.md
  3. Ensure that the rendered output contains only four paragraphs, and does not emit a "structural equivalence" warning.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • [ n/a ] Updated documentation if necessary

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@@ -347,6 +347,11 @@ public struct MarkupFormatter: MarkupWalker {

/// The current line number.
var lineNumber = 0

/// The "effective" line number, taking into account any queued newlines which have not been printed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding – what value does the lineNumber property provide? It seems confusing to have both of these now. I wouldn't be sure which to use in a given scenario. Maybe we can expand the docs of lineNumber to explain where it's useful? It seems like its doc comment, "The current line number", is maybe not accurate?

(I'm wondering if we should rename it to something like lastLineNumberWithContent but that's a little verbose and I imagine would cause a fair bit of churn in the repo...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the implementation, the lineNumber property is meant to represent the last line of printed content, and line breaks are queued up to be printed the next time non-blank content is printed. (I'm not sure the precise reason for this design decision, but it may have something to do with ensuring that line prefixes and leading indentation are correctly processed? This code is relatively old so i'd have to dig to find the original author.)

I'll update the doc comment to reflect the way lineNumber and effectiveLineNumber are used.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Sources/Markdown/Walker/Walkers/MarkupFormatter.swift Outdated Show resolved Hide resolved
Co-authored-by: Ethan Kusters <ekusters@apple.com>
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus QuietMisdreavus merged commit 3d4b36c into apple:main Jul 20, 2023
2 checks passed
@QuietMisdreavus QuietMisdreavus deleted the line-break-inline-code branch July 20, 2023 18:06
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.

None yet

2 participants