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

feat(markdown): support fenced codeblock lang followed by attributes #4153

Conversation

ikatyang
Copy link
Member

Fixes #4150

  • support ```js {something=something}
  • does not support ```js{something=something}

@@ -13,7 +13,9 @@ function embed(path, print, textToDoc, options) {
const node = path.getValue();

if (node.type === "code") {
const parser = getParserName(node.lang);
// only look for the first string so as to support [markdown-preview-enhanced](https://shd101wyy.github.io/markdown-preview-enhanced/#/code-chunk)
const lang = node.lang.split(/\s/)[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding limit to split?

const lang = node.lang.split(/\s/, 1)[0];

We only use [0] so there is no point in extracting all words from the string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure 👍

@kachkaev
Copy link
Member

@ikatyang sorry for a bit of off-topic, but since we're got code embedding here, do you know how a language parser can know that what it's formatting is a part of markdown, not a standalone file? Is there any flag getting passed down the tree that could help detect that? Just asked about this on gitter 🤔

@ikatyang
Copy link
Member Author

Should be options.parentParser, though there's no docs for it.

parentParser: parentOptions.parser,

@ikatyang ikatyang merged commit 0c09e15 into prettier:master Mar 15, 2018
@ikatyang ikatyang deleted the feat/markdown-fenced-codeblock-lang-attributes branch March 15, 2018 16:25
@kachkaev
Copy link
Member

kachkaev commented Mar 15, 2018

Thank you for this hint @ikatyang! Indeed, parentParser was there! 🎉

function parse(text, parsers, opts) {
  const isCodeBlockInMarkdown = opts.parentParser === 'markdown';

  // ...
}

Really looking forward to see the result of this PR on npm! 😍 When about is Prettier team planning to cut a new release?

@kachkaev
Copy link
Member

kachkaev commented Jun 28, 2018

@ikatyang could you recall a reason why we decided not to support js{something=something}?

Atom turns on syntax highlighting for code blocks even without spaces, which makes things a bit confusing four us in litvis:

screen shot 2018-06-28 at 22 08 13

When it is visible that the code's language has been detected, there is an expectation that it will be formatted by Prettier too, but this does not happen.

If I change the regexp to something like /^[a-z0-9_-]+/i and cover the new case with a test, would you accept this?

@ikatyang
Copy link
Member Author

ikatyang commented Jun 29, 2018

GitHub does not support it:

  • ```js{something=something}
    const x = 1;
    ```
    const x = 1;
    
  • ```js {something=something}
    const x = 1;
    ```
    const x = 1;

I won't object such change, though I think whitespace is more non-controversial.

ikatyang pushed a commit that referenced this pull request Jul 2, 2018
#4153 made it possible to detect fenced code block language when it is followed by arguments (e.g. ` ```js {something=something} `). This PR makes it also possible to detect language in cases cases like ` ```js{something=something} ` (no space).

The reason for this change is that Atom highlights code blocks regardless of a space after the language name, which makes users wonder why the correctly detected code block is not being formatted:

<img width="324" alt="screen shot 2018-06-28 at 22 08 13" src="https://user-images.githubusercontent.com/608862/42060780-db11f5b6-7b1f-11e8-9f43-fe91843f7d89.png">

PR background: #4153 (comment)
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Sep 27, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants