simplistix / sybil

Automated testing for the examples in your documentation.
https://sybil.readthedocs.io/en/latest/
Other
74 stars 14 forks source link

Parse and export directive options, too #92

Closed sscherfke closed 11 months ago

sscherfke commented 1 year ago

Status quo

ReST/MyST directives can have options, e.g.:

.. code-block:: python
   :caption: example.py

   print("Hello, world!")
'``{code-block} python
:caption: example.py

print("Hello, world!")
'``

The options are already part of the directive regex (rest, myst (although myst use the --- form and not the double colon form which seems to be recommended by MyST).

These options are not part of a capture group, though, and can thus not be used for more advanced stuff.

Use case

I have examples that show a config file, a python script that loads and prints the settings and a console code block that invokes the script and displays its output (similarly to doctests, but for bash). Here is an example: https://typed-settings.readthedocs.io/en/latest/#example

I use the :caption: filename.ext option for directives to indicate that the contents of a code block should be written to that file instead of being executed (that option also has the nice side-effect of being displayed in the docs :)).

For this to work I needed to copy and adjust lexers and parsers from Sybil. This is generally okay for me, but maybe parts of that functionality (or all of it) might be a nice addition to Sybil.

Here is the current code for this: https://gitlab.com/sscherfke/typed-settings/-/blob/main/conftest.py

Proposed changes

  1. Modify the code-block regexes to also capture the directive parameters/options.
  2. Modify the MyST code-block regex to use the double-colon format (because that is what seems to be recommended) (in addition or instead of the --- format).
  3. Modify the lexer(s) to process the options string and convert it to a dictionary (e.g. ,options = {"caption": "example.py"}) and make them part of LexedRegion.lexemes.
  4. Maybe add an options property to Region which is a shortcut to region.lexemes.get("options", {}).
  5. Add a hook (or something) to CodeBlockParser that can be used to trigger user defined functionality (e.g., depending on the directive's parameters). The current behavior is the default and fallback behavior.

If you are interested in adding this to Sybil, I would start working on a PR.

cjw296 commented 1 year ago

I think I've got a reasonable amount of this done already on https://github.com/simplistix/sybil/compare/master...skip_improvements Can you have a look and let me know what you think?

sscherfke commented 1 year ago

I skimmed through the changes.

They seem to fix my requirements 1–3 (parse :key: value options from myst/rest directive and add them to the lexemes).

The evaluator stack seems to solve my requirements for 5: I can instantiate a CodeBlockParser for console or toml (or the PythonCodeBlockParser) and push an evaluator that looks for :caption: file.ext and raises a NotEvaluated if it's not there, which would cause the default evaluators (e.g., doctest evaluator for PythonCodeBlock) to kick in. Is that correct?

cjw296 commented 1 year ago

So, coming back to this testfixtures supports what you're after but with different spelling. It's a great library, but then I'm biased since I'm its author ;-)

I think master supports what you're after, but I'm not sure I'd use the evaluator stack, just a specialised CodeBlockParser.

Can you have a look at current master and let me know if you still feel anything is missing?

cjw296 commented 1 year ago

@sscherfke - last call on this, I'm planning a 6.0.0 release sometime in the next day or few...

sscherfke commented 1 year ago

I’ll try to test the new changes and give you feedback, but had no time for this yet.

sscherfke commented 1 year ago

Wasn't there directive option parsing for the MyST lexers, too, in the feature branch skip_improvements? In the current master, it's only there for ReST.

It would be cool if that feature was also available for Myst. This is the regex I am currently using:

MYST_START_PATTERN = (
    r"^(?P<prefix>[ \t]*)"
    r"```\{(?P<directive>code-block)}\s*"
    r"(?P<arguments>[\w-]+\b)$\n"
    r"(?P<options>(?:\s*\:[\w-]+\:.*\n)*)?"
)
cjw296 commented 12 months ago

No, but you're right that it's missing and I've implemented in https://github.com/simplistix/sybil/commit/89d3eee3082dd37fc55192e9b1093e2374ec0b9d

Could you have a look at master now and see if that meets all your needs?

cjw296 commented 11 months ago

@sscherfke - I'll take the thumbs up as "all good now", please let me know if you spot anything else and we can re-open this.

sscherfke commented 11 months ago

It's a thumbs up for "I’ll test it and report back" ;-)

sscherfke commented 11 months ago

Okay, I found two issues:

  1. I think the options need to be forwared to the Region so that they are accessible in a parser. It needs to be added here: https://github.com/simplistix/sybil/blob/master/sybil/parsers/abstract/codeblock.py#L56 Like this (but as an actual class attribute ;-)):

                   r = Region(lexed.start, lexed.end, lexed.lexemes['source'], self.evaluate)
                   r.options = lexed.lexemes.get("options", {})
                   yield r
    
  2. The options in myst blocks must be enclosed with ---, but the MyST docs seem recommend options without ---: https://myst-parser.readthedocs.io/en/latest/syntax/roles-and-directives.html – I posted a regex that is able to parse this above.
sscherfke commented 11 months ago

I can provide a PR as base, if you like :)

cjw296 commented 11 months ago
  1. Both styles are already supported: https://github.com/simplistix/sybil/blob/93eb470221d089f0915db02a29887494306fef2f/tests/samples/myst-lexing-directives.md?plain=1#L21-L38
cjw296 commented 11 months ago
  1. https://github.com/simplistix/sybil/commit/f6d8772a3f0b32c79a0b4cdd83d21098cb0721a7 shows how I'd currently do the kind of thing you're after, but I'm now wondering in LexedRegion actually needs to exist...
sscherfke commented 11 months ago
  1. I need to access the options in self.evaluate(), so overriding self.__call__() as in that test example does not work for me, I guess: https://gitlab.com/sscherfke/typed-settings/-/blob/main/conftest.py?ref_type=heads#L213
  2. You're right. A mistake on my side :)
cjw296 commented 11 months ago

I'm not sure doing what you're doing in self.evaluate() is right here; you're essentially picking between two evaluators based on an option in the ReST or MyST directive, so you should code it as such rather than trying to do both in a since evaluate method.

However, as of https://github.com/simplistix/sybil/commit/59045ff9c0888106dea4ea19df019da98578f6a2 I think you can do either. Let me know what you think.

sscherfke commented 11 months ago

Hi Chris, I got sick last week. Will try your changes asap and provide feedback.

cjw296 commented 11 months ago

Very sorry to hear that, hope you're feeling better soon! 6.0.0 is out now with all the changes in it, so please give it a spin and it me know how it goes :-)

sscherfke commented 11 months ago

I am happy now (and healthy again). :-)

Thanks a lot for your efforts! <3