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
Markdown: Preserve multiple spaces in inline code #13590
Conversation
@@ -1,5 +1,162 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|||
exports[`commonmark-0.30-example-328.md - {"proseWrap":"always"} format 1`] = ` |
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.
Context: #13591
Hmmm Example 331 fails second pass:
[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 |
example 331 & example 330 has different value, prettier/src/language-markdown/clean.js Line 36 in 62d95b3
|
Pressed the wrong button 😅 I included #11373. I still don’t fully understand the relationship between |
return { | ||
...node, | ||
value: | ||
value.startsWith(" ") && value.endsWith(" ") && value.trim().length > 0 |
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.
Something doesn't look right here. Is this part really needed? Isn't this check and processing done by the parser already?
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.
Okay, I see why you did this. But this should be done in print
, not in preprocess
.
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.
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.
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.
Those tests are run only with proseWrap: "always"
, that's why it works. Need to think what to do.
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.
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:
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 <alexander@kachkaev.ru>
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 job! Just a few unimportant questions.
@@ -31,7 +31,7 @@ function transformInlineCode(ast, options) { | |||
return node; | |||
} | |||
|
|||
return { ...node, value: node.value.replace(/\s+/g, " ") }; | |||
return { ...node, value: node.value.replace(/\n/g, " ") }; |
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.
Maybe we can move this to print
? So we can reduce one mapAst
call. Can be separate PR.
@@ -109,7 +109,7 @@ markdown\` | |||
|
|||
=====================================output===================================== | |||
markdown\` | |||
const cssString = css\\\` background-color: \\$\\{color('base')\\}\\\`; | |||
const cssString = css\\\` background-color: \\$\\{color('base')\\}\\\`; |
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.
Shouldn't this call markdown -> js -> css
and remove the leading space? Because it's not valid css?
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.
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.
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.
Ah, you explained before... forgot. Sorry.
Description
Fixes #13485
Checklist
(If changing the API or CLI) I’ve documented the changes I’ve made (in thedocs/
directory).changelog_unreleased/*/XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨