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

✨ NEW: Do not allow blank lines within $$ #8

Merged
merged 5 commits into from
Jun 29, 2022
Merged

Conversation

rowanc1
Copy link
Member

@rowanc1 rowanc1 commented Jun 22, 2022

From @eric-wieser:

This matches the behavior of LaTeX (and of the stackexchange and github markdown parsers), and prevents runaway parsing if a $$ is opened but not closed.

This matches the behavior of LaTeX, and prevents runaway parsing if a $$ is opened but not closed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
rowanc1 added 2 commits June 22, 2022 14:32

Unverified

This user has not yet uploaded their public signing key.
@rowanc1
Copy link
Member Author

rowanc1 commented Jun 22, 2022

@eric-wieser, I fixed up the linting and the formatting in one test, but it looks like there is an issue with:

$$
a
$$ (label)

Which I believe should still work. Maybe this is just the label part?

@eric-wieser
Copy link
Contributor

That test is from

const tokens = mdit.parse("$$\na\n$$ (label)", {})

Right?

src/index.ts Outdated
@@ -304,14 +304,17 @@ function math_block_dollar(
start = state.bMarks[nextLine] + state.tShift[nextLine]
end = state.eMarks[nextLine]
if (end - start < 2) {
continue
break // blank lines are not allowed within $$
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is wrong, it fails on lines consisting of a single character. I think I assumed that \n was counted in the total.

Copy link
Contributor

@eric-wieser eric-wieser Jun 22, 2022

Choose a reason for hiding this comment

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

I think the entire if can be safely removed, as the new if replaces it

Copy link
Member Author

Choose a reason for hiding this comment

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

That does fix the tests. I will push and then get a sanity check from @chrisjsewell.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update on this? I'd like to port a documentation tool for the Lean programming language from using mistletoe to useing markdown-it, but can't do so without this fix!

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't heard from @chrisjsewell in the last week. I did ping him on slack. I unfortunately don't have npm admin rights to update this at the moment.

Co-authored by: @eric-wieser
Copy link
Member Author

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

This moves the if statement based on empty text rather than a token length, which is a bit more robust to other cases, which are now added.

Looks all good from my perspective and passes tests!

@rowanc1
Copy link
Member Author

rowanc1 commented Jun 22, 2022

Thanks for your contribution here @eric-wieser!

@rowanc1 rowanc1 requested a review from chrisjsewell June 28, 2022 17:27
@chrisjsewell
Copy link
Member

@eric-wieser
Copy link
Contributor

Is that something you port manually, or something that these is a script to port?

@chrisjsewell
Copy link
Member

Heya, no just manually

eric-wieser added a commit to eric-wieser/mdit-py-plugins that referenced this pull request Jun 29, 2022
@rowanc1 rowanc1 deleted the feat/blank-lines branch July 3, 2022 19:08
@eric-wieser
Copy link
Contributor

@rowanc1, would you be able to take a look at executablebooks/mdit-py-plugins#46? I was hoping to use this from python, but the two versions are still not in sync.

@rowanc1
Copy link
Member Author

rowanc1 commented Mar 3, 2023

Looks like it just needs a deploy, right?!

Yep I can test it and get it out! 🚀

@rowanc1
Copy link
Member Author

rowanc1 commented Mar 3, 2023

Oh, I see looks like you need the merge in the python side -- @chrisjsewell any chance that you can give this a bit of attention or guidance on how to bring it in?

chrisjsewell pushed a commit to executablebooks/mdit-py-plugins that referenced this pull request Mar 6, 2023
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

3 participants