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

Remove multiline math environments #233

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

vmartinv
Copy link
Contributor

This change allows removing math environments correctly when they are displayed across several lines or in a single line, i.e.

\[
  x + y = 3
\]
or
\[x + y = 3\]

The current implementation has a bug described below. It is also using replaceAll but by default it doesn't match across several lines. The flag (?s) enables multiline matching. This should solve the bug, and the issues #215 and #227.

Debugging

To discover what was happening I added some prints in the function removeEnvironments for the case in issue215.tex:

line is end of environment: \[\int_{1}^{2}x \mathrm{d}x}\]
line is start of environment:  \begin{tikzpicture}
line is end of environment:  \end{tikzpicture}
line is start of environment:  \[
line is end of environment:  \]
line is end of environment:  \[\int_{1}^{2}x \mathrm{d}x}\]

As you can see the line is detected as end of environment but not as a start, causing a mess. The reason for this is that the regex for isEnvironmentStart is not matching when it should. So there are actually two solutions:

  • The one I'm proposing here. I find this approach more simple.
  • Modifying isEnvironmentEnd to also not match for single line equations. But then \( \) would need fixing as well.

@vmartinv
Copy link
Contributor Author

Not sure why that other test is failing. I can't reproduce it locally and it seems unrelated, maybe it is flaky?

@sylvainhalle
Copy link
Owner

Indeed, it is peculiar that the build fails only for Java 17. Moreover, the failure is on a test that seems completely unrelated to your changes. Let me look into this tomorrow.

@vmartinv
Copy link
Contributor Author

I was able to reproduce it in docker locally. The tests are failing due to timeout, is not related to this diff. Maybe we can increase the timeout? This particular test has already a timeout set (code)

@sylvainhalle
Copy link
Owner

I agree, let's just increase the timeout!

@sylvainhalle sylvainhalle merged commit 0ab8b8e into sylvainhalle:master Oct 23, 2023
@vmartinv vmartinv deleted the multiline_math branch October 24, 2023 10:17
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