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: Port abbr plugin (executablebooks#14) #63

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jessicah
Copy link

No description provided.

@welcome
Copy link

welcome bot commented Feb 17, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@jessicah
Copy link
Author

Hmm, pre-commit seems broken due to PyCQA/isort#2077? I certainly can't run it.

@chrisjsewell
Copy link
Member

Thanks @jessicah, could you perhaps add some more test cases, e.g. interaction with other syntax elements: abbr definitions in lists, abbr references in link texts, ...

Looking at this now, one issue related to myst-parser I'd note, is that the extension is doing a "look-ahead" parse to find all the abbreviation definitions.
This is pain in relation to nested parsing. For example:

```{note}
*[HTML]: HyperText Markup Language

HTML
```

HTML

Here, in MyST-Parser, the outer HTML would not be converted to an abbreviation, because at that point in the rendering, the inner contents of the note have not yet been parsed.

The "ideal" solution may be, to have the markdown-it plugin collect all the abbreviation definitions, but not do the replacement of the text.
Then, after the text has been fully converted to sphinx/docutils nodes, and all the abbr definitions have been collected, run a sphinx/docutils transform to do all the find/replace replacements

@jessicah
Copy link
Author

@chrisjsewell hmm, that's how the original plugin does the replacements, it's basically a 1:1 translation of the source, just some janky translation of their regex loop.

I don't really know enough to be able to adapt to a nested parsing model; I pretty much relied on comparing an existing JS <-> Python extension... if you have some guidance, that would help!

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Base: 92.67% // Head: 92.43% // Decreases project coverage by -0.25% ⚠️

Coverage data is based on head (838f199) compared to base (4601e15).
Patch coverage: 88.46% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   92.67%   92.43%   -0.25%     
==========================================
  Files          29       32       +3     
  Lines        1679     1784     +105     
==========================================
+ Hits         1556     1649      +93     
- Misses        123      135      +12     
Flag Coverage Δ
pytests 92.43% <88.46%> (-0.25%) ⬇️

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

Impacted Files Coverage Δ
mdit_py_plugins/abbr/index.py 88.34% <88.34%> (ø)
mdit_py_plugins/abbr/__init__.py 100.00% <100.00%> (ø)
mdit_py_plugins/__init__.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chrisjsewell
Copy link
Member

if you have some guidance, that would help!

I think just for now, if you could add in an option to not make the replacements (i.e. just extract the definitions).
Then we have the option to do this on the myst-parser side.

def abbr_plugin(md: MarkdownIt, replace = True):
    """Plugin ported from
    `markdown-it-abbr <https://github.com/markdown-it/markdown-it-abbr>`__.

    .. code-block:: md
            *[HTML]: HyperText Markup Language
    """

    md.block.ruler.before(
        "reference", "abbr_def", abbr_def, {"alt": ["paragraph", "reference"]}
    )
    if replace:
        md.core.ruler.after("linkify", "abbr_replace", abbr_replace)

and yeh also if you can add some more tests

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