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(auto-render): add option to ignore escaped special characters outside of LaTeX math mode (#437) #3798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lnasrc
Copy link

@lnasrc lnasrc commented Mar 30, 2023

feat(auto-render): add option to ignore escaped special characters outside of LaTeX math mode (#437)

What is the previous behavior before this PR?
Some escapable characters were not properly escaped in math LaTeX mode in auto-render.
For instance, in the following LaTeX string:
$$
\$100 + \$100 = \$200
$$
the "\$" should be parsed as "$" symbols.

What is the new behavior after this PR?
This PR introduces a new option to auto-render:

  • supportEscapedSpecialCharsInText: boolean (default: false). If true, \$ are ignored outside of LaTeX math expressions.
    For example: Please enjoy this 2\$ coffee and I'll explain why $e = mc^2$.

When set to true, it fixes the issue and properly escape those LaTeX escapable characters.
When set to false, the existing behavior is kept to avoid breaking existing uses.

The tests have been updated accordingly, and some tests added to show those fixed behaviors.

@edemaine
Copy link
Member

I disagree with this premise (assuming I understand it correctly):

For instance, in the following LaTeX string:

$$
$100 + $100 = $200
$$

the "$" should be parsed as "$" symbols.

In fact, $s have a specific meaning inside LaTeX math, which is to re-enter math mode inside \text. For example:

$$
\{x \mid x \text{ is divisible by $y$ or $z$}\}
$$

So you can't just escape them.

Please enjoy this 2\$ coffee should already be ignored by auto-render, though #3775 is a possible improvement to that.

@lnasrc
Copy link
Author

lnasrc commented Mar 31, 2023

I disagree with this premise (assuming I understand it correctly):

For instance, in the following LaTeX string:

$$
$100 + $100 = $200
$$

the "$" should be parsed as "$" symbols.

In fact, $s have a specific meaning inside LaTeX math, which is to re-enter math mode inside \text. For example:

$$
\{x \mid x \text{ is divisible by $y$ or $z$}\}
$$

So you can't just escape them.

Please enjoy this 2\$ coffee should already be ignored by auto-render, though #3775 is a possible improvement to that.

Thanks for your reply. There was an issue with the formatting in my previous answer, where the backslashes had been removed.
I have edited my answer to fix the formatting.
Sorry for that!!

Moreover, the PR I propose does not bring any breaking change, and solve this issue mentioned in #3775:

The only way existing users are affected by this is if they have a backslash immediately (no whitespace or anything) followed by a delimiter that enters math mode.

@lnasrc lnasrc marked this pull request as ready for review March 31, 2023 07:58
@edemaine
Copy link
Member

Ahh, now I understand. Thanks for explaining! That does sound useful.

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #3798 (2b2ec31) into main (3d5de92) will increase coverage by 0.01%.
The diff coverage is 96.77%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3798      +/-   ##
==========================================
+ Coverage   92.99%   93.00%   +0.01%     
==========================================
  Files          91       91              
  Lines        6779     6803      +24     
  Branches     1576     1586      +10     
==========================================
+ Hits         6304     6327      +23     
- Misses        437      438       +1     
  Partials       38       38              
Files Coverage Δ
contrib/auto-render/auto-render.js 82.19% <100.00%> (+3.15%) ⬆️
contrib/auto-render/splitAtDelimiters.js 98.24% <94.44%> (-1.76%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d5de92...2b2ec31. Read the comment docs.

@LNA-h
Copy link

LNA-h commented Nov 20, 2023

This PR is ready for final review, thanks in advance.
How can I update the Codecov report, based on the latest commit?

@jyc
Copy link

jyc commented Feb 14, 2024

Thanks for making this! Would also be interested in helping get this over the finish line if any reviewers are willing to take a look.

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.

None yet

4 participants