Markdown: Preserve multiple spaces in inline code by kachkaev · Pull Request #13590 · prettier/prettier

kachkaev · 2022-10-08T19:11:42Z

Description

Checklist

I’ve added tests to confirm my change works.
~~(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).~~
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

kachkaev · 2022-10-08T20:22:19Z

@@ -1,5 +1,162 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

+exports[`commonmark-0.30-example-328.md - {"proseWrap":"always"} format 1`] = `


Context: #13591

kachkaev · 2022-10-08T20:38:11Z

Hmmm Example 331 fails second pass:

FULL_TEST=true yarn test markdown/spec/jsfmt

[space][space]``[space][space]
↓
[space]``[space]
↓
``

I don’t understand this because Example 331 after formatting is Example 330 (which works). Going to take a break now. Any thoughts?

UPD: I reverted changes in src/language-markdown/print-preprocess.js locally and commonmark-0.30-example-331.md failed the same way. I moved this file to a subfolder called remark-bug, which seems to be a convention for such cases. Not sure why, but example 331 works well in the Playground (it worked before too).

fisker · 2022-10-09T09:57:30Z

example 331 & example 330 has different value,

prettier/src/language-markdown/clean.js

Line 36 in 62d95b3

newObj.value = ast.value.replace(/[\t\n ]+/g, " ");

kachkaev · 2022-10-09T20:58:48Z

Pressed the wrong button 😅

I included #11373. I still don’t fully understand the relationship between src/language-markdown/print-preprocess.js → transformInlineCode and src/language-markdown/clean.js 🤔 The latter does not have access to options.proseWrap, but tests still pass. If I remove any transforms from either function, some snapshots fail 🤔

kachkaev · 2022-10-13T09:48:31Z

@thorn0 I merged next into my branch, so PR playground includes #13606 now. Thanks for implementing ‘show preprocessed AST‘!

Do you foresee any other changes we need to make before merging?

thorn0 · 2022-10-13T17:15:30Z

+    return {
+      ...node,
+      value:
+        value.startsWith(" ") && value.endsWith(" ") && value.trim().length > 0


Something doesn't look right here. Is this part really needed? Isn't this check and processing done by the parser already?

Okay, I see why you did this. But this should be done in print, not in preprocess.

I see, thanks for the fix! 🙌

Do you know why we still need newObj.value = ast.value.replace(/\n/g, " "); in clean.js? This transform is applied regardless of proseWrap. I vaguely remember trying to remove it but tests failed.

Those tests are run only with proseWrap: "always", that's why it works. Need to think what to do.

No, I was wrong. It's some other tests that fail if I add

run_spec(import.meta, ["markdown"], { proseWrap: "preserve" }); run_spec(import.meta, ["markdown"], { proseWrap: "never" });

to jsfmt.spec.js, not related to code spans.

As for clean.js, see how the clean function (AKA massageAST) is used:

prettier/src/cli/format.js

Lines 137 to 169 in 955553b

if (context.argv.debugCheck) {

const pp = prettier.format(input, opt);

const pppp = prettier.format(pp, opt);

if (pp !== pppp) {

throw new errors.DebugError(

"prettier(input) !== prettier(prettier(input))\n" + diff(pp, pppp)

);

} else {

const stringify = (obj) => JSON.stringify(obj, null, 2);

const ast = stringify(

prettier.__debug.parse(input, opt, /* massage */ true).ast

);

const past = stringify(

prettier.__debug.parse(pp, opt, /* massage */ true).ast

);

/* istanbul ignore next */

if (ast !== past) {

const MAX_AST_SIZE = 2097152; // 2MB

const astDiff =

ast.length > MAX_AST_SIZE || past.length > MAX_AST_SIZE

? "AST diff too large to render"

: diff(ast, past);

throw new errors.DebugError(

"ast(input) !== ast(prettier(input))\n" +

astDiff +

"\n" +

diff(input, pp)

);

}

}

return { formatted: pp, filepath: opt.filepath || "(stdin)\n" };

}

It's applied to both the original AST and to the AST built from Prettier's output, and then the ASTs are checked for deep equality. So it makes sense that this replacement is needed. After it, no matter what proseWrap is, the values in the two ASTs are equal.

Co-authored-by: Alexander Kachkaev <[email protected]>

fisker

Good job! Just a few unimportant questions.

fisker · 2022-10-14T01:15:45Z

    }

-    return { ...node, value: node.value.replace(/\s+/g, " ") };
+    return { ...node, value: node.value.replace(/\n/g, " ") };


Maybe we can move this to print? So we can reduce one mapAst call. Can be separate PR.

fisker · 2022-10-14T01:17:29Z

 =====================================output=====================================
 markdown\`
-  const cssString = css\\\` background-color: \\$\\{color('base')\\}\\\`;
+  const cssString = css\\\`  background-color: \\$\\{color('base')\\}\\\`;


Shouldn't this call markdown -> js -> css and remove the leading space? Because it's not valid css?

This is a crazy test. const cssString = ... is Markdown here, not JS. The Markdown-in-JS embedder also includes logic for ignoring indentation (no idea what's the story behind that). That's why the output is like that.

Ah, you explained before... forgot. Sorry.

kachkaev · 2022-10-14T09:57:45Z

Great, thanks for helping with this PR @thorn0 and @fisker! 👏

kachkaev added the lang:markdown Issues affecting Markdown label Oct 8, 2022

kachkaev changed the base branch from main to next October 8, 2022 19:14

fisker reviewed Oct 8, 2022

View reviewed changes

Comment thread src/language-markdown/print-preprocess.js Outdated

kachkaev mentioned this pull request Oct 8, 2022

Markdown spec examples need updating in tests #13591

Closed

kachkaev commented Oct 8, 2022

View reviewed changes

kachkaev added 4 commits October 8, 2022 21:24

Markdown: Preserve multiple spaces in inline code

f94f5a8

Add changelog entry

c32ebba

Improve newline handling

afd962e

Add more CommonMark examples to tests

15aee10

kachkaev requested a review from fisker October 8, 2022 20:38

kachkaev mentioned this pull request Oct 8, 2022

Muliple NBSPs in code secontion in markdown string are collapsed #13485

Closed

Move commonmark-0.30-example-331.md to remark-bug

d5fdb2c

kachkaev marked this pull request as ready for review October 8, 2022 22:22

Tweak changelog entry

e0f2ed2

kachkaev added 2 commits October 9, 2022 12:44

Keep leading & trailing spaces

8d23538

Update blog post

4fc4693

thorn0 reviewed Oct 9, 2022

View reviewed changes

Comment thread src/language-markdown/print-preprocess.js Outdated

thorn0 self-requested a review October 9, 2022 13:32

kachkaev added 2 commits October 9, 2022 21:16

Merge remote-tracking branch 'origin/next' into inline-code-spacing

dbe851d

Handle options.proseWrap, add test case

6af20a1

kachkaev mentioned this pull request Oct 9, 2022

markdown: Preserve inline code line breaks if --prose-wrap=preserve #11373

Merged

4 tasks

Move test file

feee9dd

kachkaev closed this Oct 9, 2022

kachkaev reopened this Oct 9, 2022

Merge remote-tracking branch 'origin/next' into inline-code-spacing

17f18ad

thorn0 reviewed Oct 13, 2022

View reviewed changes

thorn0 added 2 commits October 13, 2022 22:47

Add spaces in print, not in preprocess

9a7cd17

Fix adding padding for string with backticks

e2abead

kachkaev commented Oct 13, 2022

View reviewed changes

Comment thread changelog_unreleased/markdown/13590.md Outdated

Update changelog_unreleased/markdown/13590.md

5c7d4e1

Co-authored-by: Alexander Kachkaev <[email protected]>

This was linked to issues Oct 13, 2022

Markdown: Preserve non-breaking spaces in inline code #6402

Closed

Markdown: do not remove leading & trailing no-break whitespaces in inline code blocks #3755

Closed

Add tests for proseWrap: preserve

6489a4d

thorn0 linked an issue Oct 13, 2022 that may be closed by this pull request

Muliple NBSPs in code secontion in markdown string are collapsed #13485

Closed

Refactor

c5677d0

thorn0 approved these changes Oct 13, 2022

View reviewed changes

thorn0 added 4 commits October 14, 2022 01:43

Follow spec more closely

0856e6a

Follow spec even more closely

0b42cdf

Add a test

f939b6d

Fix one more case

33d7a43

fisker approved these changes Oct 14, 2022

View reviewed changes

Remove preprocess, move logic to print

5350af8

thorn0 merged commit 8df3b5e into prettier:next Oct 14, 2022

This was referenced Oct 14, 2022

Markdown: Preserve non-breaking spaces in inline code #6402

Closed

Markdown: do not remove leading & trailing no-break whitespaces in inline code blocks #3755

Closed

kachkaev deleted the inline-code-spacing branch October 14, 2022 09:58

sosukesuzuki mentioned this pull request Nov 12, 2022

2.8 blog post #13813

Merged

1 task

medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 6, 2024

Markdown: Preserve multiple spaces in inline code (prettier#13590)

ecdef2c

		@@ -1,5 +1,162 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`commonmark-0.30-example-328.md - {"proseWrap":"always"} format 1`] = `

	if (context.argv.debugCheck) {
	const pp = prettier.format(input, opt);
	const pppp = prettier.format(pp, opt);
	if (pp !== pppp) {
	throw new errors.DebugError(
	"prettier(input) !== prettier(prettier(input))\n" + diff(pp, pppp)
	);
	} else {
	const stringify = (obj) => JSON.stringify(obj, null, 2);
	const ast = stringify(
	prettier.__debug.parse(input, opt, /* massage */ true).ast
	);
	const past = stringify(
	prettier.__debug.parse(pp, opt, /* massage */ true).ast
	);

	/* istanbul ignore next */
	if (ast !== past) {
	const MAX_AST_SIZE = 2097152; // 2MB
	const astDiff =
	ast.length > MAX_AST_SIZE \|\| past.length > MAX_AST_SIZE
	? "AST diff too large to render"
	: diff(ast, past);
	throw new errors.DebugError(
	"ast(input) !== ast(prettier(input))\n" +
	astDiff +
	"\n" +
	diff(input, pp)
	);
	}
	}
	return { formatted: pp, filepath: opt.filepath \|\| "(stdin)\n" };
	}

Uh oh!

Conversation

kachkaev commented Oct 8, 2022 • edited by thorn0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kachkaev commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker commented Oct 9, 2022

Uh oh!

Uh oh!

kachkaev commented Oct 9, 2022

Uh oh!

kachkaev commented Oct 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fisker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kachkaev commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kachkaev commented Oct 8, 2022 •

edited by thorn0

Loading

kachkaev commented Oct 8, 2022 •

edited

Loading

thorn0 Oct 13, 2022 •

edited

Loading