-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Removed excessive spaces after line prefixes for unordered lists in Markdown #15526
Conversation
5645761
to
1b94310
Compare
Hello @fisker, what can I do to push this PR forward? |
Hi, |
Any progress on this? I would really like to see this feature! |
@TomasLudvik @fisker what's the status on this PR? This is the one problem preventing Prettier adoption at our org. |
@glenn-jocher It is waiting for review from @fisker or @sosukesuzuki as I am not a maintainer of this repo. |
Hi @fisker, @sosukesuzuki |
Hi @fisker, @sosukesuzuki, I really hate to pester, but this is causing some immense grief with having to manually remove the extra spaces after the markers, is there anything anyone can do to push this forward? The deviation fights with basically every example and other linting tool out there as shown in a comment in #5019 which makes it a near impossible sell to match the other way round. |
@fisker @sosukesuzuki please merge this PR as soon as you can without further delay. |
Prettier messes up unordered lists due to prettier/prettier#5019, pending a fix in prettier/prettier#15526
* Copy-edited POSH policy * Remove extra spaces * Ignore Markdown files in Prettier Prettier messes up unordered lists due to prettier/prettier#5019, pending a fix in prettier/prettier#15526
@fisker @sosukesuzuki another bump for getting this PR merged 🙏 |
I'm not familiar with the markdown spec, but will this break CommonMark? |
I'm not sure about this change TBH. If I understand it correctly, it essentially ignores {
"tabWidth": 4, // prerequisite
"overrides": [
{
"files": "*.md",
"options": {
"tabWidth": 2
}
}
]
} That said, I’m not certain enough to say that this PR is wrong. I’m just confused about it’s best to ignore |
@fisker @kachkaev guys the #5019 bug that this PR fixes has 130 upvotes in #5019 (comment), many many users are facing problems that this PR fixes. The basic problem is that |
@fisker @sosukesuzuki @kachkaev friendly bump to please merge this. Our 80k-star organization is pending this PR to apply prettier to all our 300 MkDocs pages, it would really help us a lot. Currently we are considering installing prettier directly from @TomasLudvik fork instead, but this is much slower, i.e. |
It's here, and it works! Thanks everybody, real life saver this one 👍 |
The merge of this change is surprising. Maybe I'm missing something, but there is a clear distinction between:
and
The first list item contains a text item with the text I know you can argue about this all year around, and point to different specifications and implementations as the single source of truth, but I'm shocked to see how many people were in favor of this. If you follow the original spec, you will find this section: The spec also says:
This dictates these two options: Prettier 3.4.0
Prettier 3.3.1
Now when you use something like Prettier, you want your lists and texts to look nice. The spec literally says "To make lists look nice, you can wrap items with hanging indents". This would indicate to me that hanging indents are also the sane default for Prettier. So when I indent with 4 spaces, as is the Markdown standard, then why wouldn't my list item labels be offset from their list item markers? I'm also not sure if the effects of |
Here https://www.markdownguide.org/basic-syntax/#paragraphs is described your use case exactly as it prettier does now, so I think the current behavior is the correct one. |
This is a core Markdown behavior of how to nest HTML block elements in lists. Markdown tooling can still render this code to your expectation for your convenience, but it's still in conflict with the spec. It is important to note that the output you receive from compiling a Markdown document to HTML is not what should decide Prettier behavior. Prettier prettifies source code, which is the Markdown document itself. If you introduce a change that modifies the semantics of the document, and then argue based on rendered output, you're misleading reviewers. |
@oliversalzburg I am really confused by your explanation. First you mention the original spec that fixates 4 spaces, in the next comment you say that prettier should follow commonmark that has a different interpretation. Now the rule of commonmark seems to be to that any additional paragraphs or blocks within a list item must be indented to align with the start of the text in the first line. This is what prettier does now: it reduces the spaces, but also adjust the indentation of subsequent blocks in the list item to match that. So Prettier does seem to follow the common spec, or am I missing something? |
@MartenBE Let me try to elaborate.
|
@oliversalzburg I have introduced this change because it solves problems for 90% of users, as many do not use paragraphs in the lists. I am sorry that it caused you some issues. The fastest way to solve this is to find the fix for the indentation of paragraphs in the lists and create a pull request to solve this once and for all. |
@TomasLudvik I feel like I left the wrong impression. I'm really somewhat neutral on the subject. However, with all due respect, I doubt you have the metrics on the user base to make any assessments like "90% of users are affected", and I believe that I have clearly illustrated some aspects of Markdown and Prettier that warrant additional consideration in this context. I'm not trying to invalidate the original requirement for this change. I just doubt that this is the correct change to satisfy the requirements while still staying within the spec. I can see that a lot of users were frustrated about this behavior for over a year, with hardly any response from maintainers. How the threads read to me, maintainers were just tired and wanted to get this off their dashboard, without investing any time into the specifics of the issue - just like they had been for a year before finally merging this change. I don't blame you for suggesting a change that seems obvious so that, at least, a process can get started to find the right solution. I also fully understand that, after a year of feeling ignored, you don't have the motivation to backtrack on any of this; neither would I. This change in Prettier brought up code style violations in pretty much all of my JavaScript projects, but I don't care about reformatting all my I also maintain projects that parse Markdown and interpret the semantic blocks in the AST to generate new information. That's the only reason why my attention moved here. This change in Prettier introduces document transformations that are not compliant with the spec that Prettier aims to support. This makes 100% of the documents produced by Prettier liable to be non-compliant to the spec, which overrules your 90% 😉 I wish I could offer an olive branch to find some middle ground for certain cases where the "excessive spaces" could actually be removed, but I'm having a hard time seeing how the implementation complexity and implicit unreliable behavior would actually be a benefit to users. I'm also having a hard time even understanding the original requirements. All of them are purely personal interpretation of esthetics. The only real requirement is that "it would make markdownlint happy". markdownlint is highly opinionated and even explains directly in its README that the default rules are probably wrong for you. If you're not even customizing your markdownlint rule set, based on the recommendations of the author, why would Prettier need to change? Again, I'm not trying to invalidate that users feel a pain, I just don't understand their situation fully. When I read through the original threads, everybody who complains is saying things like:
Okay. Why? What is your real problem that requires us to make Prettier non-compliant with CommonMark? |
@oliversalzburg Just my 2 cents. A lots of the users come to this PR because of #5019, personally I don't know which of the following complies with the specs, but intuitively I like the 2nd case because it would be the same as I write the markdown manually (only typing one space after the list marker). With tab width 4 Before this PR - Item
- Subitem
- Subitem
- Item
- Subitem
- Subitem After this PR - Item
- Subitem
- Subitem
- Item
- Subitem
- Subitem In the 2nd case, the tab width doesn't affect the spaces after the list markers. The discussions above are mainly around the subsequent paragraph
Maybe some of the users, especially the ones come from #5019 (like me), didn't realize the impact of subsequent paragraph, and mainly voted for the issue and this PR because of the spacing after the list markers. Personally, I don't know too much about the spec, and only care if the prettier output looks nice to me, and if the render HTML looks as expected. In some of my projects with tab width set to 4 (for markdown and source code), this PR fix the looking issue, for the other projects with tab width 2, with or without this PR is the same, although using tab width 2 might already be non-compliant with the markdown spec. I just want to share the original problem and the feeling of an end user, and maybe there is a middle ground to make prettier compliant with the specs, as well as making the users like me happy about the spaces after the list marker. Or maybe the requirements of #5019 doesn't compliant with the specs at all when the tab width is 4, then it has to be a trade off, meet the spec or make the users happy. |
@CHC383 Thank you for the clear reflection of a user's original mindset that brings up this requirement. I really do appreciate it. Right out of the gate, I have to agree that the after example looks dramatically more aesthetically pleasing than the first one. There is no argument about this from my point of view. But given the wide variety of Markdown documents I've seen on GitHub alone, this is not a realistic example that illustrates the issue fully. Now, if you were using an indent of 2 spaces in your configuration, we wouldn't even have this argument, right? Because then you would have had the after result even before the change. So why is 4 spaces even a subject, if 2 spaces provides a more suitable result already? We have to remember that when Markdown was thought up, it recognized tabs as an equal option to spaces in the indentation mechanics. I don't want to go to into the thinking behind that, but I hope one example illustrates the core issue here. Prettier and IDE set to 2 Space Indent
Here, I have started an ordered list and pressed The result is not a nested list; it's two list siblings:
Prettier and IDE set to 2 Space Indent - How Users fix itThere are 2 common approaches users use to resolve this: 3 Space Indent Fix
Some users like to insert the missing space (manually or automatically), which aligns the layout with the spec. As long as you don't introduce 2-digit list item prefixes, you have no problems with this solution. Users who prefer this solution also commonly like to not declare the position of their list items explicitly. Instead of using Once you hit 2-digit indices, the layout of the Markdown source and the rendered result become inconsistent and unpleasant.
renders as
Prettier and IDE set to 4 Space IndentAnd this is exactly why the safe default is 4 spaces. This gives you plenty of room for list indices that go up to 99.
Here, I have started an ordered list and pressed The result is a nested list, as was my intention. However, the Markdown looks somewhat less pleasing than with the 3 space indent. Also, this is basically abusing the fact that the single space before the new ordered list block element is ignored when the Markdown is rendered. From a parser's point of view there is, and always was, a space in front of the nested, ordered list - unless I would add another space after the Why is this person talking about ordered lists, when this is about unordered lists? Unordered lists obviously have the benefit that their list item indicator never changes in length; it's always Ultimately, it doesn't matter which type of list you are nesting inside the list element. The semantics that dictate the space between the list item indicator and its label are the same.
Code BlocksI know that we all love to use triple-backtick "code fences" or whatever you want to call them today. But Markdown dictates support for purely indent-indicated code blocks. If you look at the original spec, you'll see that 4 spaces, or 1 tab, also indicate a code block. CommonMark also still fully recognizes indent-based code blocks (check out example 109). Now try to combine the concepts of indent-indicated code blocks with indent-indicated list item nesting. This is one of the biggest challenges in Markdown since the original spec. The OP in #5019 explicitly stated:
Yes, @borekb, this is non-obvious if there ever was something non-obvious. I find this very hard to tie together, but I hope this playground helps to illustrate it. When you carefully examine this, you will hopefully see confusing inconsistencies. If you modify the spacing between list item indicators, and the list item label, you will also see how the interpretation of the nested code blocks changes. Remember, a nested list block does not suffer from invalid list item Markdown syntax, because excessive spaces have no effect on the block. But when you declare a nested code block through indentation, then every single space becomes relevant. You can see this when you render the Prettier output from the above playground to HTML- code blocks easily have every single line prefixed by a single space. So what's the Solution?Sorry for taking so many words to say "You're wrong. Get over it.". What you are seeing is a fragment of applying Markdown/CommonMark correctly. Ultimately, Markdown is a language to describe HTML. While we want a beautiful Markdown document when we apply Prettier, we also want that document to follow patterns that reflect robust best practices. If what you are aiming for only looks good as long as you have only a single line on each list item, and you never expect anyone to want to use indent-based code blocks, that's fine, but maybe that's not a great sane default for a tool as widely used as Prettier. I feel like the authors of Prettier originally understood this and that's why we had the original implementation. The introduced change is a regression due to miscommunication and maintainer fatigue. |
One reason for using 4 spaces is that some implementation require 4 spaces for nested list items, see e.g. Python-Markdown/markdown#451 (this library is also used by mkdocs). |
We can discuss all we want here, but I think this is the way forward. The unit tests of prettier didn't catch this, so then it must be included. Best indeed to create a PR to fix it and discuss it there. |
We have -- [About](#about)
-- [Help](#help)
-- [Build](#build)
- - [Library](#library)
- - [Examples](#examples)
- - [Tests](#tests)
-- [Test](#test)
- - [Build and Run](#build-and-run)
- - [Run (Only)](#run-only)
+- [About](#about)
+- [Help](#help)
+- [Build](#build)
+ - [Library](#library)
+ - [Examples](#examples)
+ - [Tests](#tests) this conflicts with https://github.com/markdownlint/markdownlint/blob/main/docs/RULES.md#md007---unordered-list-indentation MD007:
indent: 4
start_indented: false
start_indent: 4
MD030:
ul_single: 3
ol_single: 2
ul_multi: 3
ol_multi: 2 The idea of 4-sized indents is that the list (either numbered or unordered) starts at first position and has 3 more characters (needed for numbered lists when they run into teens, e.g. |
@Alexander-Shukaev The link you gave points further to https://cirosantilli.com/markdown-style-guide/#spaces-before-list-marker for 4 space indents, which actually advises the prettier corrected output for one line items in unordered lists:
For the other case (multiparagraph), there was already some discussion, but there is need for someone to step up and create a PR that handles both this case and the paragraphs in lists as said in #15526 (comment) |
@MartenBE, my point is solely that the style, for which I gave my rationale and we relied upon, is now broken by a hard-coded change. I personally find that advice about single-line items highly debatable and the damage done by the current change far worse for the general case than the "benefit" of having inconsistent style for one-liners, however nobody has to agree with me and that's fine (as that's what we have configuration options for). As it currently stands, this PR is disruptive and low-quality change and, with all the respect, it should be considered for a revert to be instead fixed properly in future for all cases (but without disruptions to existing consistent 4-space style). I disabled Prettier for Markdown in our organization until then (or forever). |
Note that this change only changes a single line of business code, and makes 270 changes to test code to make the change pass. A statement like, "unit tests of prettier didn't catch this" is completely misplaced in this thread IMHO. |
Such a statement is borderline insane, given that markdownlint explicitly states that it is extremely opinionated, and you most likely need your own rules. Like, it's right there directly on the front page README of the tool. Why are you even referencing this? |
The only thing that has become clear, since I started to contribute to this thread, is how useless of a tool Prettier actually is today. We argue here about a core aspect of the tool, with zero maintainers in sight. And they are the ones who accepted this completely invalid change in the first place. Prettier is a joke. If you forced it to work your way now, I'm happy for you. But the times where it was a sane default tool is over. I'm still happy to use the last working versions and am very grateful for all the work of all the contributors that made that version possible. But there is no fun in following the development further |
I meant that this implies that we should add unit/integration tests to see if the formatting still generates the same HTML with commonmark.js, to prevent this incertainty in the future. If I have some time, I think this is a cool experiment:
If the generated HTML output is the same by the commonmark.js library, this whole discussion is moot. If there are differences, then it forms a nice base for a new PR. |
So, I found some time and did the test as described in my previous comment in light to put this discussion back on things we can objectively measure. I compared the outputs of the existing tests from v3.3.3 and v3.4.2 by these files as these are the only edited test files in the PR according to https://github.com/prettier/prettier/pull/15526/files#diff-983b3030349454a8de0fac39f7936183afd84febe286948e2f26c7bf5500ba13:
Where the outputs where different (thus edited in the PR), I put those outputs through the CommonMark Dingus ( https://spec.commonmark.org/dingus/ ) to convert to HTML and then diffed the outputted HTML snippets to see if there are any structural differences between v3.3.3 and v3.4.2. These are the results:
16 unit tests where edited in this PR. And, it seems that the only difference is the word wrapping, which is logical as some spaces are omitted due to this PR ( So I think we can conclude that according to the unit tests, this PR didn't include any structural breaking changes according to the CommonMark norm. If something still isn't right, new tests and fixes should be added like we said before by creating a new issue or PR. |
@Alexander-Shukaev I found this PR ( #4281 ), which I think could be very useful in your use case and also be more future proof (aligning lists with 100+ items etc.). Perhaps it is best to revive that PR as it is still open and labeled |
Holy shit, who thought that this breaking change was a good idea? Using prettier 3.3.3:
Using prettier 3.4.0:
Note the second paragraph in the first list item using more spaces for indentation when using prettier 3.4.0. Wen fix? |
CommonMark wise they output the same HTML, but I agree the latter is nicer to look at. The conondrum is that the indent of Perhaps it would be best to let prettier see what you do in the first entry and just apply that to the rest of the list, that way everybody could be happy. Let's discuss this further in your issue at #16929 . |
Including the reformatting changes needed to make it pass again (mostly due to prettier/prettier#15526).
Including the reformatting changes needed to make it pass again (mostly due to prettier/prettier#15526).
does anyone know how long until this surfaces in the vscode prettier extension? |
See https://github.com/prettier/prettier-vscode?tab=readme-ov-file#prettier-resolution, the extension uses the version you installed. This is in Prettier 3.4.0 |
Description
This change fixes excessive spaces after line prefixes for unordered lists in Markdown. Closes #5019.
Checklist
docs/
directory).changelog_unreleased/*/XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨