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

(WIP) Myst parser: .md + {eval-rst} support #31

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

Conversation

tony
Copy link
Contributor

@tony tony commented Jul 12, 2022

re: #30

Naïve eval-rst directive support.

@tony tony changed the title Myst parser (part 1): .md + {eval-rst} support (WIP) Myst parser: .md + {eval-rst} support Jul 12, 2022
@tony
Copy link
Contributor Author

tony commented Jul 12, 2022

@Thisch I will ping you when this is ready to look at

Have not hand tested yet. I will build a very robust testsuite for it

@twmr
Copy link
Owner

twmr commented Jul 12, 2022

Awesome! Thx a lot.

@tony
Copy link
Contributor Author

tony commented Jul 12, 2022

@Thisch feel free to let tests run

@tony tony force-pushed the myst-parser branch 3 times, most recently from d8c8662 to 938f0a4 Compare July 12, 2022 13:19
@twmr
Copy link
Owner

twmr commented Jul 12, 2022

@tony thx a lot for all your PRs. I've just enabled the CI pipeline.

@tony tony force-pushed the myst-parser branch 3 times, most recently from 173f4da to 3f98b3f Compare July 12, 2022 20:09
@tony
Copy link
Contributor Author

tony commented Jul 12, 2022

@Thisch Thank you!

Current PR status: I need to test this by hand, but open to any early feedback

@tony tony force-pushed the myst-parser branch 2 times, most recently from adf4213 to 6e88ca8 Compare July 12, 2022 21:46
@tony
Copy link
Contributor Author

tony commented Jul 12, 2022

@Thisch I suppose if you want you could cut a 0.4.1 release with the latest regex changes (via #33, #34)

I assume this PR, if merged (I still need to test it by hand and QA it more intensively) will be a bigger version bump, e.g. 0.5.0/0.6.0

Copy link
Owner

@twmr twmr left a comment

Choose a reason for hiding this comment

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

I'll try to give you more feedback on the weekend.

pytest_sphinx.py Outdated
@@ -71,6 +71,7 @@ def _is_doctest(config, path, parent):

_DIRECTIVE_RE = re.compile(
Copy link
Owner

Choose a reason for hiding this comment

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

_DIRECTIVE_RE.match is currently used for every line in a docstring/txt file.

pytest_sphinx.py Outdated
@@ -71,6 +71,7 @@ def _is_doctest(config, path, parent):

_DIRECTIVE_RE = re.compile(
r"""
(?P<myst_directive>`{3}{eval-rst}\n?)?
Copy link
Owner

Choose a reason for hiding this comment

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

Therefore, this line, doesn't currently add change anything.

If you want that _DIRECTIVE_RE finds (for markdown content) a match for the following input

```{{eval-rst}}
.. docstest::

but not for

```{{xxxxx}}
.. doctest::

we have to extend the code that calls _DIRECTIVE_RE.match

Copy link
Contributor Author

@tony tony Jul 15, 2022

Choose a reason for hiding this comment

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

I've been having some issues when testing this locally in my project, so this may be why.

I will look into this.

(note to self, the above message refers to how the regex checks every line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Thisch

Could you take a stab at getting it to work? (using the existing parser)

I am going try a fork of this branch that uses the myst-parser/docutils/etc API.

Here's what you can do:

I added a test to tests/test_sphinx_doctest.py::TestDirectives::test_doctest. It automatically skips if myst-parser is not found

pip install myst-parser or pip install -e .[myst-parser] (bash) or pip install -e ".[myst-parser]" (zsh)

Then py.test

Re-invoke on file rerun:

pip install pytest-watcher

With this test

py.test tests/test_sphinx_doctest.py::TestDirectives::test_doctest ; ptw . tests/test_sphinx_doctest.py::TestDirectives::test_doctest

Project-wide

py.test -x -s -v; ptw . -x -s -v

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Thisch I did a lot of research this week, I am going to be digging into the docutils, sphinx, and myst / markdown-it and pytest APIs next weekend. This is priority 0 for me, outside of work

@tony tony force-pushed the myst-parser branch 2 times, most recently from 145ca1a to 79de46d Compare July 17, 2022 21:42
@tony tony force-pushed the myst-parser branch 7 times, most recently from 7857cf0 to e3f674b Compare September 7, 2022 00:52
@tony
Copy link
Contributor Author

tony commented Sep 7, 2022

@Thisch I may take a fresh stab at this non-docutils PR now that I am familiarized with doctest 👍

This approach, if pulled it off, would be easier to merge, preserve existing features, and have 0 extra dependencies.

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