-
Notifications
You must be signed in to change notification settings - Fork 193
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
add indentationStyle to MarkupFormatter.Options #204
base: main
Are you sure you want to change the base?
Conversation
… code accordingly
Could you add a CLI flag to the format tool in |
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.
Good PR, but a couple more things:
- The indentation for content nested underneath a block directive (line 488) uses a four-space indent; i don't think we handle tabs for nested block-directive content, but if that is handled that should also be updated.
- Can you add some tests in
MarkupFormatterTests
?
prefix += formattingOptions.indentationStyle.indentation() | ||
} | ||
unorderedListCount += 1 | ||
} else if element is OrderedList { | ||
if orderedListCount > 0 { | ||
prefix += " " | ||
prefix += formattingOptions.indentationStyle.indentation() |
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.
These indentation markers use different numbers of spaces between ordered and unordered lists to align trailing text with the previous line. While it may still parse correctly, this is a regression in behavior.
Your comment about block directives has sent me down a bit of a rabbit
hole.
* The word "directive" doesn't appear in the commonmark spec (that I
can see), and some poking at the web and LLMs gives me the sense
that block directives are an extension.
* I'd be happy to tabify/abstract the indentation in the code as part
of the PR... but.
* The commonmark spec claims: "Tabs in lines are not expanded to
spaces <https://spec.commonmark.org/0.31.2/#space>. However, in
contexts where spaces help to define block structure, tabs
behave as if they were replaced by spaces with a tab stop of 4
characters."
* the spec also claims that continuation paragraphs need to be
indented four spaces.
* the spec also allows up to 3 spaces of indentation in many cases
without changing the meaning of a line start (such as headers)
but goes on to say the 4 spaces is too many and will change the
meaning.
* Indented code blocks says: An indented chunk
<https://spec.commonmark.org/0.31.2/#indented-chunk> is a
sequence of non-blank lines, each preceded by four or more
spaces of indentation.
* This goes on and on and generally implies that 4 spaces is the
magic number.
* That would imply that I should replace the 4 spaces in the code with
a single tab.
* But also, lists are weird and special.
* From the spec: "The most important thing to notice is that the
position of the text after the list marker determines how much
indentation is needed in subsequent blocks in the list item. If
the list marker takes up two spaces of indentation, and there
are three spaces between the list marker and the next character
other than a space or tab, then blocks must be indented five
spaces in order to fall under the list item."
* But in all other places in the swift-markdown code a 2 space indent
has been chosen.
I see a few possible solutions:
1. Replace these 4 spaces with a single tab if indentationStyle is set
to tab and reduce it to two spaces if indentationStyle is set to
spaces. This would be more consistent with other indentation choices
made in the code, but would cause rendering differences with older
versions of the code.
2. Replace the 4 spaces with a single tab but hard code 4 spaces if
spaces is set. This feels clunky and like it's going to confuse
someone a lot later. But it's backwards compatible.
3. Change `case spaces` to be `case spaces(int)` and use that many
spaces in all places. This doesn't really solve the backwards
compatibility issue, but it does let the user dictate the space count
in all places. This is kind of 1 with a bit of extra space choice.
If it were my code alone, I would switch to 4 spaces as a hard coded
indent in all places as a start. It might be the case that the list
indenting code needs more poking to be fully compliant, but in reality
it probably works 99% of the time and any changes should be a different
PR.
Since it's not my code alone, I'll let you tell me what you'd prefer.
:)
David (he/him)
On October 10, 2024, GitHub ***@***.***> wrote:
@QuietMisdreavus requested changes on this pull request.
Good PR, but a couple more things:
1. The indentation for content nested underneath a block directive
(line 488) uses a four-space indent; i don't think we handle tabs
for nested block-directive content, but if that is handled that
should also be updated.
2. Can you add some tests in MarkupFormatterTests?
In Sources/Markdown/Walker/Walkers/MarkupFormatter.swift
<https://github.com/swiftlang/swift-
markdown/pull/204#discussion_r1795696090>:
> + prefix += formattingOptions.indentationStyle.indentation() }
unorderedListCount += 1 } else if element is OrderedList { if
orderedListCount > 0 { - prefix += " " + prefix +=
formattingOptions.indentationStyle.indentation()
These indentation markers use different numbers of spaces between
ordered and unordered lists to align trailing text with the previous
line. While it may still parse correctly, this is a regression in
behavior.
—
Reply to this email directly, view it on GitHub
<#204 (review)-
2360764538>, or unsubscribe
<https://github.com/notifications/unsubscribe-
auth/AAGINYGEH7IPSE72CIUW4B3Z22N6HAVCNFSM6AAAAABPW2ZLMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNRQG43DINJTHA>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
…nd make that more uniform in the emission code
Summary
Add indentationStyle (.tabs/.spaces) to MarkupFormatter.Options to allow format to use tabs when outputting Markdown.