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

👌 IMPROVE: MyST role syntax parsing #32

Merged
merged 1 commit into from
Dec 2, 2021
Merged

Conversation

chrisjsewell
Copy link
Member

Match characters instead of using regex.
This now allows for an unlimited name length and new lines in the content.

Match characters instead of using regex.
This now allows for an unlimited name length and new lines in the content.
@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #32 (2e312e1) into master (fc130fa) will decrease coverage by 0.16%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   93.20%   93.04%   -0.17%     
==========================================
  Files          24       24              
  Lines        1237     1251      +14     
==========================================
+ Hits         1153     1164      +11     
- Misses         84       87       +3     
Flag Coverage Δ
pytests 93.04% <84.21%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mdit_py_plugins/myst_role/index.py 85.36% <84.21%> (-3.53%) ⬇️

Continue to review full report at Codecov.

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

Copy link

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

How about adding a test case with a line break in between the backticks to cover executablebooks/MyST-Parser#448?

<p>a <code class="myst role">{abc}[xyz]</code> b</p>
.

New lines:
Copy link
Member Author

Choose a reason for hiding this comment

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

@webknjaz that test is already here. Is there any other fixtures you can think of before I merge

Copy link

Choose a reason for hiding this comment

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

How about a more complex case with spaces in the reference between angled braces?

{ref}`some
text
<and
a
custom reference>`

Copy link
Member Author

@chrisjsewell chrisjsewell Dec 2, 2021

Choose a reason for hiding this comment

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

thanks for the quick reply

This complexity won't particularly make a difference here though: the markdown-plugin just stores everything inside the ticks as a raw string (now replacing new lines with spaces), then the token (with name+content) is processed specifically by myst-parser. i.e. it doesn't really matter what is inside the ticks (in the example above the token's content will be some text <and a custom reference>)

Copy link

Choose a reason for hiding this comment

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

Got it. But I'd still have some regression test for this, if not in this lib, then in some other place maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I can add a test like this in myst-parser, when I downstream this change to there 👍

@chrisjsewell chrisjsewell merged commit 65f73e0 into master Dec 2, 2021
@chrisjsewell chrisjsewell deleted the update_myst_role branch December 2, 2021 15:36
@asmeurer
Copy link

asmeurer commented Dec 3, 2021

Does (or could) the unlimited length thing also apply to executablebooks/MyST-Parser#181?

@chrisjsewell
Copy link
Member Author

Does (or could) the unlimited length thing also apply to executablebooks/MyST-Parser#181?

yep indeed 👍, thank for pointing that out

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.

3 participants