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

use myst parser for myst and rest for rest #2

Closed
wants to merge 3 commits into from

Conversation

cjw296
Copy link
Contributor

@cjw296 cjw296 commented Jul 13, 2023

@hynek
Copy link
Owner

hynek commented Jul 13, 2023

first sorry for the force pushes, I wasn't aware someone is gonna open a PR against this repo 😅

(if you have constructive feedback beyond eww, feel free to chime in 😇)


That said, your changes explode in hilarious ways :D I think this might be the point where I begged you to not parse all code blocks by default, months ago? 😅 The code examples, at this point, are not supposed to be executable. But it's also complaining about inline code blocks? 😅

@hynek hynek force-pushed the main branch 9 times, most recently from ddcbad7 to ad93ee7 Compare July 13, 2023 10:42
@cjw296
Copy link
Contributor Author

cjw296 commented Jul 14, 2023

No worries, I read the warning in the readme, I just wanted to see if I could fix the Sybil config.
I have my own dependency injection thing which I was trying to make work for FastAPI, but no time to work on it. I also don't use flask, so probably not the target audience here 😅

I think this might be the point where I begged you to not parse all code blocks by default, months ago? 😅

Can you point me to this discussion? I think I took everything into account, so it's hopefully just a case of configuration.

The code examples, at this point, are not supposed to be executable.

Which code examples are those? Should be easy enough to exclude them with configuration.

But it's also complaining about inline code blocks? 😅

Can you point me to an example of this?

@cjw296
Copy link
Contributor Author

cjw296 commented Jul 14, 2023

Might be worth re-running the CI on this, I just did a 5.0.3 release of Sybil that fixes the traceback trimming on the latest release of pytest.

@hynek
Copy link
Owner

hynek commented Jul 15, 2023

I'm busy touring Prague pre-EP rn so just briefly: I've re-run the tests and they still fail. Given the the errors, probably because they're parsing code that's not a doc test.


I got my thing working with FastAPI:

from contextlib import asynccontextmanager

from fastapi import Depends, FastAPI, Request
from typing_extensions import Annotated

from svc_reg import Container, Registry


@asynccontextmanager
async def lifespan(app):
    registry = Registry()

    registry.register_value(str, "HELLO")

    app.registry = registry

    yield


app = FastAPI(lifespan=lifespan)


async def svc_container(request: Request):
    return Container(request.app.registry)


@app.get("/")
async def read_users(container: Annotated[Container, Depends(svc_container)]):
    return [{"registry gave us": container.get(str)}]

not sure it's the best way tho, because I don't use it myself.


My next plan is to switch from explicit clean up methods to allowing to yield the resource like in pytest fixtures and cleanup later. But the usefulness of this is only really obvious when you start having 3 or more services that you need in different combinations.

@cjw296
Copy link
Contributor Author

cjw296 commented Jul 16, 2023

Cool, but let's keep this PR focussed on getting Sybil working for you. Once these questions get answered, I'm hopeful we can get it sorted:

I think this might be the point where I begged you to not parse all code blocks by default, months ago? 😅

Can you point me to this discussion? I think I took everything into account, so it's hopefully just a case of configuration.

The code examples, at this point, are not supposed to be executable.

Which code examples are those? Should be easy enough to exclude them with configuration.

But it's also complaining about inline code blocks? 😅

Can you point me to an example of this?

@hynek
Copy link
Owner

hynek commented Jul 22, 2023

Sorry for the slow responses, EuroPython has been a thing!

So I'm incapable to unfuck the situation that I've caused for you with my force pushes, so I've just applied the changes to a local branch and am running it locally.

I think this might be the point where I begged you to not parse all code blocks by default, months ago? 😅
Can you point me to this discussion? I think I took everything into account, so it's hopefully just a case of configuration.

I wish I could remember where we talked about it, but either way the examples that aren't supposed to be executed fail because they're using sqlalchemy and I don't want to install sqlalchemy.

FAILED README.md::line:53,column:1 - ModuleNotFoundError: No module named 'sqlalchemy'
FAILED README.md::line:109,column:1
FAILED README.md::line:173,column:1 - ModuleNotFoundError: No module named 'sqlalchemy'
FAILED README.md::line:229,column:1 - NameError: name 'app' is not defined
FAILED README.md::line:237,column:1 - NameError: name 'app' is not defined
FAILED README.md::line:324,column:1 - ModuleNotFoundError: No module named 'your_app'
FAILED README.md::line:335,column:1 - ModuleNotFoundError: No module named 'your_app'
FAILED README.md::line:353,column:1 - NameError: name 'Connection' is not defined

What I'd like is to say to only run doctests and ignore normal code.

The code examples, at this point, are not supposed to be executable.
Which code examples are those? Should be easy enough to exclude them with configuration.

Essentially all that aren't doctests. :) See the line numbers above.

But it's also complaining about inline code blocks? 😅
Can you point me to an example of this?

OK it doesn't seem to do it anymore, sorry! It's a lot of output.

@hynek
Copy link
Owner

hynek commented Jul 22, 2023

the current patch looks like this btw if you wanna rebase/force push:

diff --git README.md README.md
index 359d240..e845430 100644
--- README.md
+++ README.md
@@ -51,6 +51,8 @@ The latter already works with [Flask](#flask).
 You set it up like this:
 
 ```python
+from sqlalchemy import create_engine
+
 engine = create_engine("postgresql://localhost")
 
 def engine_factory():
diff --git conftest.py conftest.py
index c704a59..b4b4ded 100644
--- conftest.py
+++ conftest.py
@@ -7,18 +7,35 @@ from doctest import ELLIPSIS
 import pytest
 
 from sybil import Sybil
-from sybil.parsers.rest import DocTestParser, PythonCodeBlockParser
+from sybil.parsers.myst import DocTestDirectiveParser as MarkdownDocTestParser
+from sybil.parsers.myst import (
+    PythonCodeBlockParser as MarkdownPythonCodeBlockParser,
+)
+from sybil.parsers.rest import DocTestParser as ReSTDocTestParser
+from sybil.parsers.rest import (
+    PythonCodeBlockParser as ReSTPythonCodeBlockParser,
+)
 
 import svc_reg
 
 
-pytest_collect_file = Sybil(
+markdown_examples = Sybil(
     parsers=[
-        DocTestParser(optionflags=ELLIPSIS),
-        PythonCodeBlockParser(),
+        MarkdownDocTestParser(optionflags=ELLIPSIS),
+        MarkdownPythonCodeBlockParser(),
     ],
-    patterns=["*.md", "*.py"],
-).pytest()
+    patterns=["*.md"],
+)
+
+rest_examples = Sybil(
+    parsers=[
+        ReSTDocTestParser(optionflags=ELLIPSIS),
+        ReSTPythonCodeBlockParser(),
+    ],
+    patterns=["*.py"],
+)
+
+pytest_collect_file = (markdown_examples + rest_examples).pytest()
 
 
 @pytest.fixture(name="registry")

@cjw296 cjw296 closed this Jul 22, 2023
@cjw296 cjw296 deleted the patch-1 branch July 22, 2023 10:04
@cjw296
Copy link
Contributor Author

cjw296 commented Jul 22, 2023

It's weird to only want to test half the examples in your documentation. What's your plan for making sure there aren't bugs or doc-rot in your non-doctest python code blocks?

(MyST doesn't make what you're asking for easy, if they'd gone for ```doctest instead of just mushing it in with ``python :-()

@cjw296
Copy link
Contributor Author

cjw296 commented Jul 22, 2023

Also, why don't you want to install sqlachemy for your documentation testing? If you're using it your docs, how do you intend to check that your examples keep working as new versions of sqlalchemy are released?

@hynek
Copy link
Owner

hynek commented Jul 22, 2023

I don't want it necessarily forever, but currently the project is experimental and I don't think it's that weird to show real-life examples using heavy packages that I don't want the tests to depend on?

Also, why don't you want to install sqlachemy for your documentation testing? If you're using it your docs, how do you intend to check that your examples keep working as new versions of sqlalchemy are released?

If I decide this to make a proper project, I'll have a separate tox environment for these kinds of tests, but I've had cases where unnecessary test dependencies had undesired effects (including but not limited to pulling in dependencies on their own which made packaging tests meaningless). This is why I've stopped installing test dependencies into the dev environment and started providing tox and Nox targets to continuously build the docs. E.g. https://github.com/hynek/structlog/blob/ec810c50e124a58ec5ecfc07bd119c7e4c87950f/tox.ini#L52-L62

Aside: README.md isn't even MyST… it's just GFM. I believe MyST just copied it from there which makes sense. The easiest way to determine a doctest would be if the first line starts with >>> , I believe?

@hynek
Copy link
Owner

hynek commented Jul 22, 2023

To be clear: those examples are currently just ads/teasers. Like there isn't even an app in the Flask examples. If I decide to make this serious, there will be proper ones.

@cjw296
Copy link
Contributor Author

cjw296 commented Jul 22, 2023

To be clear: those examples are currently just ads/teasers. Like there isn't even an app in the Flask examples. If I decide to make this serious, there will be proper ones.

For these, I'd recommend the skip parser instead:
https://sybil.readthedocs.io/en/latest/myst.html#skipping-examples

@cjw296
Copy link
Contributor Author

cjw296 commented Jul 22, 2023

Aside: README.md isn't even MyST… it's just GFM

GFM?

@hynek
Copy link
Owner

hynek commented Jul 22, 2023

GitHub-flavored Markdown. The readme only gets rendered by GitHub currently.

@hynek hynek mentioned this pull request Jul 23, 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.

2 participants