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

zmarkdown: highlight latex and keep console as plainText #264

Closed
wants to merge 1 commit into from

Conversation

artragis
Copy link
Member

@artragis artragis commented Aug 9, 2018

@artragis
Copy link
Member Author

artragis commented Aug 9, 2018

I don't know why tests are failing here.

@vhf
Copy link
Contributor

vhf commented Aug 9, 2018

Something changed in an upstream package we transitively depend on: syntax-tree/mdast-util-to-hast@742a780

This fix makes remark-parse more compliant with the commonmark spec. We could either hold this dependency back or upgrade, I think it's better to upgrade. I'll ping you here as soon as it's fixed, you'll then have to rebase on master.

@vhf
Copy link
Contributor

vhf commented Aug 9, 2018

I'll prepare this on a separate branch, I might wait on syntax-tree/mdast-util-to-hast#22 before merging it.

@vhf
Copy link
Contributor

vhf commented Aug 9, 2018

I decided to pin the dependency instead. You can now rebase on master.

@artragis
Copy link
Member Author

artragis commented Aug 9, 2018

@vhf it's rebased.

"<div class=\\"hljs-code-div\\"><div class=\\"hljs-line-numbers\\"><span></span><span></span><span></span><span></span></div><pre><code class=\\"hljs language-latex\\"><span class=\\"hljs-tag\\">\\\\<span class=\\"hljs-name\\">\\\\</span></span>usepackage{inputenc}[utf8]
<span class=\\"hljs-tag\\">\\\\<span class=\\"hljs-name\\">\\\\</span></span>begin{document}
<span class=\\"hljs-tag\\">\\\\<span class=\\"hljs-name\\">\\\\</span></span>texttt{code}
<span class=\\"hljs-tag\\">\\\\<span class=\\"hljs-name\\">\\\\</span></span>end{document}
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I read here, only the \ is highlighted, end{document} is not highlighted.

This seems wrong. TeX highlighting should at least detect {} and its content, and highlight this.

Copy link
Contributor

@vhf vhf Aug 9, 2018

Choose a reason for hiding this comment

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

If you have issues reading snapshots, you don't have to use snapshots.

Or before using a snapshot, you could check what the snapshot would be. Try .toBe(null) instead of .toMatchSnapshot(), it will usually tell you that you expected the output to be null but got instead.

Read carefully what is, and if you are 100% sure it is correct, use a snapshot.

Or use whatever trick you think of, the goal is to commit good code, not to follow a certain procedure.

@artragis
Copy link
Member Author

output was good, and in latex, but input was not good due to the fact I use template and did not escape { and }

Copy link
Contributor

@vhf vhf left a comment

Choose a reason for hiding this comment

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

There is no need to escape {} in a template string.

Please run the following code:

const dedent = require('dedent')

const input = dedent `\`\`\`latex
\\usepackage{inputenc}[utf8]
\\begin\{document\}
\\texttt\{code\}
\\end\{document\}
\`\`\``

and check that this is the input your test is supposed to test.

@artragis
Copy link
Member Author

th'ats what I did in my last update and as you can see it's ok.

@vhf
Copy link
Contributor

vhf commented Aug 10, 2018

It doesn't look right.

Why did you have to escape here: \{document\} but not here: {inputenc}?

@vhf
Copy link
Contributor

vhf commented Aug 10, 2018

Let's go back to my previous comment, I think you should check that the input of your test is correct:

const dedent = require('dedent')

const input = dedent `\`\`\`latex
\\usepackage{inputenc}[utf8]
\\begin\{document\}
\\texttt\{code\}
\\end\{document\}
\`\`\``

screen shot 2018-08-10 at 11 01 41

Is this the markdown string you want to test?

Here is the last line of your input: (a tag, a name, a tag, a name, a tag, a name)

<span class="hljs-tag">\<span class="hljs-name">\</span></span>end<span class="hljs-tag">\<span class="hljs-name">{</span></span>document<span class="hljs-tag">\<span class="hljs-name">}</span></span>

I think it should be: (a tag, a name, a string)

<span class="hljs-tag">\<span class="hljs-name">end</span><span class="hljs-string">{document}</span></span>

@artragis
Copy link
Member Author

Ok, so,

const input = dedent `\`\`\`tex
\\usepackage{inputenc}[utf8]
\\begin{document}
\\texttt{code}
\\end{document}
\`\`\``

I got the same result as when I ask for latex.

So two possibilities :

  • the output is really what we expect
  • the input is not good, but I don't get where.

@artragis
Copy link
Member Author

artragis commented Aug 10, 2018

It was the second option.

I had a look on how we test it on lowlight and rehype-highlight side and I just adapted the test with it.

It seems that I do not master the string format management in javascript. Can you explain it please?

When I did:

const input = dedent `\`\`\`latex
\\usepackage{inputenc}[utf8]
\\begin{document}
\\texttt{code}
\\end{document}
\`\`\``

the result was not the same as

const input = ['````latex',
      '\\usepackage{inputenc}[utf8]',
      '\\begin{document}',
      '\\texttt{code}',
      '\\end{document}',
      '```'].join('\n')

@vhf
Copy link
Contributor

vhf commented Aug 10, 2018

Good job, that's much better :)

See how dedent behaves like String.raw:

> const dedent = require('dedent')
> dedent(`\\`)
'\\'
/* careful here: tagged template: */
> dedent`\\`
'\\\\'
> String.raw`\\`
'\\\\'

I'll rework the tests a bit to fix the formatting, add a test, remove an extra ` here: ```` and I'll merge.

@vhf vhf mentioned this pull request Aug 10, 2018
@vhf
Copy link
Contributor

vhf commented Aug 10, 2018

Merged: bee1f54

@vhf vhf closed this Aug 10, 2018
@artragis artragis deleted the fix_highlights branch December 19, 2020 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants