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

Double empty line in markdown lists changes marker #4369

Closed
kachkaev opened this issue Apr 25, 2018 · 11 comments · Fixed by #8140
Closed

Double empty line in markdown lists changes marker #4369

kachkaev opened this issue Apr 25, 2018 · 11 comments · Fixed by #8140
Labels
lang:markdown Issues affecting Markdown locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! scope:dependency Issues that cannot be solved inside Prettier itself, and must be fixed in a dependency type:bug Issues identifying ugly output, or a defect in the program

Comments

@kachkaev
Copy link
Member

kachkaev commented Apr 25, 2018

Looks like two or more empty lines between both list items in markdown work as a flipper for the marker. * toggles to - and 1. toggles to 1) 🤷‍♀️

Prettier 1.12.1

Input:

* hello1

* hello2


* hello3

* hello4


* hello5

* hello6

Output:

* hello1

* hello2

- hello3

- hello4

* hello5

* hello6

Marker style does not fix itself even after the second format.

Expected behavior:

* hello1

* hello2

* hello3

* hello4

* hello5

* hello6
@ikatyang ikatyang added type:bug Issues identifying ugly output, or a defect in the program priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! scope:dependency Issues that cannot be solved inside Prettier itself, and must be fixed in a dependency lang:markdown Issues affecting Markdown labels Apr 25, 2018
@ikatyang
Copy link
Member

Opened remarkjs/remark#332.

@kachkaev
Copy link
Member Author

Thanks @ikatyang for opening an upstream issue. Even though remark gets three lists instead of one, what causes the markers to flip? I thought this would be the printer's job, which is a part of prettier.

@ikatyang
Copy link
Member

@kachkaev
Copy link
Member Author

Not sure I'm right, but IMHO prettier could just merge consequent lists and the problem would be solved. In which situations would this cause a regression?

I also guess there is a chance that remark is right about producing several lists in that scenario.

@ikatyang
Copy link
Member

Their ASTs are different and it'll cause rendered HTML to be different especially if CSS applied, for example:

* hello1
* hello2
- hello3
- hello4
* hello5
* hello6
  • hello1
  • hello2
  • hello3
  • hello4
  • hello5
  • hello6

@kachkaev
Copy link
Member Author

To be honest, it's hard for me to imagine a scenario when someone wants to have two lists of the same kind following each other. It's much more common that there's an extra blank line after a listing etc. inside a list item, which unexpectedly splits up the list. I have been facing this several times already and had to manually go through 1) → 1. in diffs after running Prettier.

@ikatyang
Copy link
Member

There's no marker kind in the AST, all we know in prettier is that remark told us there're three separate lists, so we cannot fix this issue without upstream fixes.

Currently remark produces the same AST for the following two cases:

* hello1
* hello2


* hello3
* hello4
* hello1
* hello2
- hello3
- hello4

@kachkaev
Copy link
Member Author

Yeah, I understand that there are two lists in the AST. What I'm wondering is if these lists could be ‘merged’ during printing. This would require a change in getPrefix(), which currently looks like this:

function getPrefix() {
const rawPrefix = node.ordered
? (index === 0
? node.start
: isGitDiffFriendlyOrderedList
? 1
: node.start + index) +
(nthSiblingIndex % 2 === 0 ? ". " : ") ")
: nthSiblingIndex % 2 === 0
? "* "
: "- ";

E.g. roughly to this:

          function getPrefix() {
            const rawPrefix = node.ordered
              ? (index === 0
                  ? node.start
                  : isGitDiffFriendlyOrderedList
                    ? 1
                    : node.start + index) + "."
              : "*";

(isGitDiffFriendlyOrderedList would just have to apply to the fist list in a sequence)

This change would not allow for two consequent lists to be rendered in the output. I know that those are supported by the spec, but I'm not sure if they are being used at all in practice. Accidental insertion of a double \n\n in a single list is way more common (at least, according to my own experience).

@kachkaev
Copy link
Member Author

What I'm trying to say is that Prettier could have its own opinion when it comes to consequent lists of a single kind. It can just merge all such lists into a single one and thus better work for cases like:

* hello1

* hello2

- hello3

- hello4

* hello5

* hello6

(I see this list as a single list, it's just not prettified)

It's probably fine that remark detects separate lists when there's too much white space between item. there is a chance that some users of that library consider this as a correct behavior.

@ikatyang
Copy link
Member

We can't merge them even if it's less used, the 1st goal for a formatter is to be correct, people won't trust a formatter that'll change the meaning of the code for any reason.

I believe this issue should be fixed in remark since the root cause is that it didn't follow the CommonMark spec in this case.

@joealden
Copy link

joealden commented Jan 7, 2020

Any movement on this problem? I am still experiencing this issue on the latest version of prettier (1.19.1) even though I can see that the "problem" was fixed upstream (remarkjs/remark#332).

@thorn0 thorn0 linked a pull request May 7, 2020 that will close this issue
4 tasks
alexander-akait pushed a commit that referenced this issue Jun 23, 2020
* Update `remark-parse` to v7

* fix `link` title and  `inlineCode`

* fix `code` meta

* fix code meta

* fix ListItem print

* fix heading print result

* remove blank line before childList

* trim `inlineCode` ast

* use List.spread

* use ListItem.spread

* fix `example-550.md` snap

* update `example-522.md` snap

* update `example-508.md` snap

* update `example-509.md` snap

* update `example-222.md` snap

* Update listitem print

* lower case definition.label to match ast

* trim definition.label to match ast

* trim definition.label to match ast

* unescape Link.title

* filter link with no title

* fix link title

* fix link title

* restore preprocess

* fix ast massage

* Restore list

* Update to remark-parse 8.0.2

* Update snapshot tests after rebase

* Create legacy `loose` attribute from `spread`

Uses @duailibe's suggestion from here:
#6180 (comment)

Also backtracks some other unnecessary changes.

* Update test case for shortcut reference-style links

The commonmark spec does not allow for whitespace between the left and right
parts of a full reference-style link.

https://spec.commonmark.org/dingus/?text=%5Bfoo%5D%20%5Bbar%5D%0A%0A%5Bbar%5D%3A%20%2Furl%20%22title%22%0A

Notice that the `[foo]` is parsed as plain text, while the `[bar]` is parsed
as a shortcut reference link.

When there is a space between the two, each part is parsed as a separate
shortcut style reference link node:

https://astexplorer.net/#/gist/d69f5a227e2cbde3f43f0a639bee165c/822b531c252c162faba78e38607db73ef72886d5

Previously they would have been parsed as a single full reference link.

Here I update the test-case to account for the shortcut style links.

* Add remark-footnotes plugin to replace legacy footnotes option

Remark removed the footnotes option in favor of a new remark-footnotes
plugin in remarkjs/remark#483

Here I've simply switched to use the plugin instead of the removed
option and updated the snapshot tests.

* Add test case for #4369

* Do not add gap if has leading space

* Restore `inlineCode` ast compare

* Restore `isPrevNodeLooseListItem`

* Ignore some ast property

* Restore blank line, Fix `isPrevNodeLooseListItem` check

* Title compare

* Ignore `loose`

* Fix footnoteDefinition

* Fix tabWidth

* Style

* `footnoteDefinition` is stable now

* Fix `_` after link

* Revert workaround, url tests

* Fix idempotence issue with underscores and autolinks

* Fix idempotence issue with underscores and autolinks - attempt # 2

* Fix idempotence issue with underscores and autolinks - attempt # 3

* Fix idempotence issue with underscores and autolinks - part # 4

* More tests

* Code style

* Only test master passed tests

* Update list

* Update list

* Use code as test name

* Fix idempotence issue with underscores and autolinks - part # 5

* Fix idempotence issue with underscores and autolinks - part # 6

* Linting fix

* More autolink tests

* Fix tests

* Rename to `auto-link`

* More tests

* Disable new cases

* tests for issue 4122

* add changelog

Co-authored-by: fisker <lionkay@gmail.com>
Co-authored-by: Georgii Dolzhykov <thorn.mailbox@gmail.com>
@github-actions github-actions bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Sep 22, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:markdown Issues affecting Markdown locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:high Code is printed in a way that alters the AST, breaks syntax, or is a significant regression. Urgent! scope:dependency Issues that cannot be solved inside Prettier itself, and must be fixed in a dependency type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants