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

Fixed index.js delimeter logic #3

Merged
merged 1 commit into from
Apr 22, 2016
Merged

Conversation

paulsohn
Copy link
Contributor

@paulsohn paulsohn commented Apr 21, 2016

  • Since the code is suggesting using only $ - $ for inline, $$ - $$ for block math syntax,
    specific delimiter logic for $ - $ and $$ - $$ is required, not general escape logic.
    Also it will fix the issue could not parse xxx$-2x<4$xxx #2.
  • Some code optimization and variable name change was done.
  • I found that 'throwOnError:false' option doesn't actually work on katex. They will still throw an error if the syntax is invalid.
    So instead of sending this directly, I fixed this code to catch the error internally, return the original latex token, and log it on the console when throwOnError option is explicitly set to true.

* Since the code is suggesting using only $ - $ for inline, $$ - $$ for block math syntax,
 specific delimiter for $ - $ and $$ - $$ is required, not general escape logic.
 also it will fix the issue waylonflinn#2.

* Some code optimization and variable name change was done.

* I found that 'throwOnError:false' option doesn't actually work on katex. They will still throw an error if the syntax is invalid.
 So instead of sending this directly, I fixed this code to catch the error internally, return the original latex token, and log it on the console when throwOnError option is explicitly set to true.
@waylonflinn
Copy link
Owner

waylonflinn commented Apr 21, 2016

Thanks! Looking this over now!

Really makes me wish we had some unit tests!

@waylonflinn
Copy link
Owner

Writing some unit tests that cover current behaviour, so we can ensure nothing will break here.

I'm excited about the patch for throwOnError. I'm hoping we can get some of those fixes pushed upstream.

@waylonflinn
Copy link
Owner

According to the new tests (adapted from markdown-it-math), this does fix #2, but it also breaks the following:

Display math self-closes at the end of document
.
$$
1+1 = 2
.
<span class="katex-display"><span class="katex"><span class="katex-mathml"><math><semantics><mrow><mn>1</mn><mo>+</mo><mn>1</mn><mo>=</mo><mn>2</mn></mrow><annotation encoding="application/x-tex">1+1 = 2
</annotation></semantics></math></span><span class="katex-html" aria-hidden="true"><span class="strut" style="height:0.64444em;"></span><span class="strut bottom" style="height:0.72777em;vertical-align:-0.08333em;"></span><span class="base displaystyle textstyle uncramped"><span class="mord mathrm">1</span><span class="mbin">+</span><span class="mord mathrm">1</span><span class="mrel">=</span><span class="mord mathrm">2</span></span></span></span></span>
.

Going to look through the code and see if this is an easy fix.

@paulsohn
Copy link
Contributor Author

paulsohn commented Apr 22, 2016

Okay, I missed that test things.
I had ignored the rules '4 indents are code block'[1] and 'opening $$ tag closes itself if there's no closing $$ tag in the end of any line after', just thinking those are no need to be here, I removed those.

Therefore, in this commit, changes of optimization and new logic of escaping inline $ sign would be valid.

[1] though I still can't understand why it is needed.

@paulsohn
Copy link
Contributor Author

My inline escaping is based on following regular expression:
(?<!\\)(?:((?<!\$)\$(?!\$)))([^\n\r]+)(?<!\\)(?(1)(?<!\$)\1(?!\$))

If only javascript regexp interpreter has a lookbehind options.

@waylonflinn
Copy link
Owner

Okay, I think I've got fixes for the broken stuff. Merging. 🎉

@waylonflinn waylonflinn merged commit 2a45cb8 into waylonflinn:master Apr 22, 2016
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