mmcky commented 2 years ago

This PR adds support for gated exercise and solution directives, which enables:

support for executable code (such as code-cell in myst-nb) to be used. As the code-cell is at the root level of the document it is executed by jupyter-cache and follows with jupyter-book methodology of supporting code-cell directives at the root level of the document in a linear fashion (which is 1-to-1 with jupyter)
supports any directives that may be used between [exercise,solution]-start and a [exercise,solution]-end directive by transforming the AST which can be a convenient way to add nested directives without needing to increase the back-tick depth

Basic Syntax

```{exercise-start}
:label: ex1

paired with


The `exercise-start` directive allows for he same options as core `exercise` directive. 

````md
```{solution-start} ex1

paired with


## Example

````md

```{solution-start} exercise-1
:label: solution-gated-1

This is a solution to Exercise 1

import numpy as np
import matplotlib.pyplot as plt

# Fixing random state for reproducibility
np.random.seed(19680801)

dt = 0.01
t = np.arange(0, 30, dt)
nse1 = np.random.randn(len(t))                 # white noise 1
nse2 = np.random.randn(len(t))                 # white noise 2

# Two signals with a coherent part at 10Hz and a random part
s1 = np.sin(2 * np.pi * 10 * t) + nse1
s2 = np.sin(2 * np.pi * 10 * t) + nse2

fig, axs = plt.subplots(2, 1)
axs[0].plot(t, s1, t, s2)
axs[0].set_xlim(0, 2)
axs[0].set_xlabel('time')
axs[0].set_ylabel('s1 and s2')
axs[0].grid(True)

cxy, f = axs[1].cohere(s1, s2, 256, 1. / dt)
axs[1].set_ylabel('coherence')

fig.tight_layout()
plt.show()

With some follow up text to the solution



will be styled as a single solution in the output `document`

<img width="707" alt="Screen Shot 2021-12-07 at 8 43 17 pm" src="https://user-images.githubusercontent.com/8263752/145005401-5cd4abc8-9dd8-4f1a-b267-182905216dc8.png">

## Technical Note

Currently the `solution` and `exercise` implementations differ in approach to try different ways of achieving the same thing. 

1. The `exercise` approach maintains the original `node` type `exercise` or `enumerate_exercise` and instead uses `gated=True/False` metadata on the class to determine if it is an `exercise` start node. The benefit of this approach is that the `content` is more easily merged as you don't need to create a `new_node` and reassemble it (as in the case of `solutions`)
2. The `solution` approach uses fully separate wrapper nodes `solution-start` and `solution-end`, however the `solution-start` node needs to be cast into a `solution` node in the transform. 

---

## Discussions around Syntax

**Pro:**

1. This approach is independent of the current `solution` directive so doesn't change any existing behaviour
4. Looks quite clean from a syntax perspective (see example below)

**Con:**

1. Harder for `highlighter` tools to parse (i.e. vscode extension)
4. There are two directives that do a `similar` thing (`solution` and `solution-start` with `solution-end`). (@mmcky comment: Not sure this is a huge issue as I think users would typically use one or the other. I would probably start using the gated directives as I like how they look (rather than nested structures).

### Technical Approach

This PR adds in two directives `solution-start` and `solution-end` which are then transformed during `transform` phase of sphinx to migrate any nodes between `start` and `end` to be wrapped in a standard `solution` node. Therefore there are no additional `visit_` and `depart_` methods required.

- [x] `solution-end` can be simplified as it currently inherits from `SolutionDirective` and doesn't require all the additional logic of titles. It just needs to plan a `solution-end` node in the tree.

codecov-commenter commented 2 years ago

Codecov Report

Merging #45 (a16787b) into master (92b9614) will increase coverage by 1.12%. The diff coverage is 96.82%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   92.98%   94.10%   +1.12%     
==========================================
  Files           6        7       +1     
  Lines         442      628     +186     
==========================================
+ Hits          411      591     +180     
- Misses         31       37       +6

Flag	Coverage Δ
pytests	`94.10% <96.82%> (+1.12%)`	:arrow_up:

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

Impacted Files	Coverage Δ
sphinx_exercise/directive.py	`97.14% <94.73%> (-1.18%)`	:arrow_down:
sphinx_exercise/transforms.py	`97.32% <97.32%> (ø)`
sphinx_exercise/__init__.py	`95.06% <100.00%> (+0.77%)`	:arrow_up:
sphinx_exercise/nodes.py	`90.32% <100.00%> (+0.78%)`	:arrow_up:

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 92b9614...a16787b. Read the comment docs.

jstac commented 2 years ago

This definitely works for me @mmcky . Fits my simple uses cases and the syntax is clean and simple enough to easily remember. Fully in favor :+1:

Oops, hit wrong button :grimacing: -- reopened.

mmcky commented 2 years ago

Thanks for comments @jstac @choldgraf (@chrisjsewell via slack). Appreciate it.

Next Steps:

[ ] do another round to improve robustness and write a more general tree parsing function for collecting relevant nodes between -start and -end.
[ ] add developer documentation

There is the possibility to make this more general to extend any directives using {{directive}}-start and {{directive}}-end but won't put development time into this at the present time. Perhaps we could revisit that for an admonition factory type of project. I will document the general approach if others want to use this pattern in an extension.

mmcky commented 2 years ago

@AakashGfude I just pushed some additional test cases error.md but with the current test setup using app.build() I don't see a way to save the errors and warnings to pass to an assert statement. Any ideas on this?

Also the way the transform works currently is:

it parses each document looking for solution-start nodes
An exception is raised if no matching solution-end is raised when finding nodes to merge into a base solution node

I don't really see a reason to do any further structural testing on the document. This condition seems to be the main issue -- and I think an exception (vs. a warning) is required so users can fix the issue in the md.

[x] incorporate edge cases listed by @choldgraf above into error testing once testing infrastructure is setup correctly

mmcky commented 2 years ago

OK I think the only way to integrate proper error checking is to:

[x] add a transform CheckGatedDirectives to run through the full structure of the document and build a registry to ensure the structure of gated directives is complete (i.e. matching -start and -end), non-nestedness etc. and raise Errors if the required structure is not adhered to.

Alternatively,

[x] the registry can be formed as the directives are parsed.

I will try the alternative first as it would be more efficient to check an already formed registry (as part of the parsing step)

The current transform then doesn't have to know the structure of the document and can focus on migrating the appropriate nodes into a unified solution note in the sphinx.ast.

psychemedia commented 2 years ago

In a simple exercise solution, the dropdown class can be added to the solution to hide it.

For compound solutions between gated sections, it would be useful if the whole section could be collapsible, perhaps by the following:

```{solution-start} solution1
:class: dropdown



More generally, can a class be set in the `solution-start` block, eg `:class: orange` for custom styling, that is then automatically removed (by default, at least) by the matching `solution-end` block.

psychemedia commented 2 years ago

Regarding supporting code in exercise directives, this can be useful where some set up or initialisation code is required as part of the exercise set-up.

Generalising support for gated directives would seem to be a good way of doing this, not least because it then opens up a route for other extension developers to easily support high level blocks defined across multiple heterogenous cells.

Naively, I'm assuming that at some point the gated directives essentially gives you a <div> </div> block to put things in; as such, it would also be useful to allow the user to optionally set a CSS id for the block.

mmcky commented 2 years ago

In a simple exercise solution, the dropdown class can be added to the solution to hide it.

For compound solutions between gated sections, it would be useful if the whole section could be collapsible, perhaps by the following:
```{solution-start} solution1
:class: dropdown
More generally, can a class be set in the `solution-start` block, eg `:class: orange` for custom styling, that is then automatically removed (by default, at least) by the matching `solution-end` block.

Thanks @psychemedia I think this is a good idea. The way solution-start is setup at the moment is it inherits the solution directive so all options available to solution nodes should transfer over to solution-start. Maybe I need to add a test case as I work through the gated nodes.

mmcky commented 2 years ago

Regarding supporting code in exercise directives, this can be useful where some set up or initialisation code is required as part of the exercise set-up.

Support for exercise nodes will be next.

mmcky commented 2 years ago

@chrisjsewell @AakashGfude I have setup the extension to make use of a CheckGatedDirectives transform before merging them in the sphinx.AST to check for various structural issues. I am not sure how to get this setup working in the test framework though -- Is it possible to catch a sphinx ExtensionError and keep executing the document.

I want sphinx to stop executing if any of these malformed gated directive conditions exist:

no matching -end
no matching -start
nested sequence of -start and -end

coupled with a useful error message.

Currently the make html presents a nice diagnostic message for tests/books/test-gateddirective/

(ebp) ➜  test-gateddirective git:(gated-directive) make html
Running Sphinx v4.4.0
myst v0.15.2: MdParserConfig(renderer='sphinx', commonmark_only=False, enable_extensions=['dollarmath'], dmath_allow_labels=True, dmath_allow_space=True, dmath_allow_digits=True, dmath_double_inline=False, update_mathjax=True, mathjax_classes='tex2jax_process|mathjax_process|math|output_area', disable_syntax=[], url_schemes=['http', 'https', 'mailto', 'ftp'], heading_anchors=None, heading_slug_func=None, html_meta=[], footnote_transition=True, substitutions=[], sub_delimiters=['{', '}'], words_per_minute=200)
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 6 source files that are out of date
updating environment: [new config] 6 added, 0 changed, 0 removed
Executing: errors_1 in: /Users/mmcky/work/executablebooks/sphinx-exercise/tests/books/test-gateddirective                               
ERROR: The document (errors_1) is missing a solution-end directive
  solution-start at line: 20

Extension error:
[sphinx-exercise] An error has occured when parsing gated directives.
Please check warning messages above
make: *** [html] Error 2

but only for error_1 and I am not sure how to test against this condition and keep executing for errors_2 and errors_3 documents.

Should I run a sphinx.build() for each of the error pages individually and catch the exception?

mmcky commented 2 years ago

OK I have just found https://github.com/executablebooks/sphinx-external-toc/blob/96c80db479b9cd926f2d4dad9cc43ee3c007fcb7/sphinx_external_toc/events.py#L69 so maybe that is tested in sphinx-external-toc. I'll look there.

psychemedia commented 2 years ago

I'm trying this from pip3 install --upgrade git+https://github.com/executablebooks/sphinx-exercise.git@gated-directive but I don't see the code cells being displayed? Is code cell inclusion between the gateposts supported yet? Mangled Jupytext header code breaking code display.

psychemedia commented 2 years ago

Should the solution content blocks all be inside a solution-content div, which would then allow for collapsing the solution content?

mmcky commented 2 years ago

Should the solution content blocks all be inside a solution-content div, which would then allow for collapsing the solution content?

thanks @psychemedia -- I will double check the html. Is this just for gated solutions?

psychemedia commented 2 years ago

Is this just for gated solutions?

Yes, that's where I spotted it. Inside a gated solution. I was expecting the content between the gateposts to be in the solution-content div. Also, looking at it now, is the id="solution-content" guaranteed unique? It looks more like a class than an id to me.

mmcky commented 2 years ago

nicely spotted thanks @psychemedia the test fixture shows the same -- will fix that.

<div class="solution admonition" id="solution-gated-1">
<p class="admonition-title"><a class="reference internal" href="exercise.html#exercise-1">Solution to Exercise 1</a></p>
<div class="section" id="solution-content">
</div>
<p>This is a solution to Exercise 1</p>

[x] fix non-nested solution div in the translator phase
[x] check if id="solution-content" should be added to class="section solution-content"

The sphinx.ast is looking properly nested so this suggests it is the translation phase rather than transforms

<solution_node classes="solution" docname="solution" hidden="False" ids="solution-gated-1" label="solution-gated-1" names="solution-gated-1" serial_number="0" target_label="exercise-1" title="Solution to" type="solution">
            <solution_title>
                Solution to
            <section ids="solution-content">
            <paragraph>
                This is a solution to Exercise 1
            <CellNode cell_type="code" classes="cell">
                <CellInputNode classes="cell_input">
                    <literal_block language="ipython3" xml:space="preserve">

psychemedia commented 2 years ago

Presumably, the gated solutions will not be useful in JupyterLab / JupyerLab-MyST contest because of the inability to propagate metadata up the DOM / select parent HTML blocks that define cells; or, at least, not without some sort of extension support, or the availability of CSS has() selectors?

psychemedia commented 2 years ago

How easy is it to be able to parse a :class: dropdown on entering the gated section, and then add the dropdown button to the solution div?

More generally, can the recipe for gated sections be used to easily create other admonition types that also employ gated sections? Eg could gated blocks be easily added to exercise definitions, or other custom defined admonition blocks based on a similar pattern?

mmcky commented 2 years ago

Presumably, the gated solutions will not be useful in JupyterLab / JupyerLab-MyST contest because of the inability to propagate metadata up the DOM / select parent HTML blocks that define cells; or, at least, not without some sort of extension support, or the availability of CSS has() selectors?

Sadly no - to my knowledge this will need extension support. There is some work going on at mystjs to bring rendering of MyST support for jupyter/jupyterlab.

mmcky commented 2 years ago

OK so it looks like the xml tree is incorrect and the solution-content section should nest the content and it should be:

<section ids="solution-content">
            <paragraph>
                This is a solution to Exercise 1
            <CellNode cell_type="code" classes="cell">
                <CellInputNode classes="cell_input">
                    <literal_block language="ipython3" xml:space="preserve">

mmcky commented 2 years ago

[x] work out how to best support error_1, error_2 and error_3 tests

mmcky commented 2 years ago

@chrisjsewell @AakashGfude I think I figure out a reasonable solution in 701a398 for testing the Exception cases and checking warning messages

mmcky commented 2 years ago

@choldgraf @psychemedia @AakashGfude this is working nicely for me now.

I'll plan to merge this soon and start work on exercise directives.

@choldgraf this extension to the syntax is quite convenient (vs. nesting structures) -- it might be a nice enhancement proposal for any directive down the track.

A real world example can be seen here:

https://6226e59b74051d4c841f8092--epic-agnesi-957267.netlify.app/functions.html#solutions

which uses sphinx-togglebutton to collapse the blocks (thanks @choldgraf)

mmcky commented 2 years ago

I have now converted one of the QuantEcon lectures sites to use sphinx-exercise that makes use of both gated exercise and solution directives https://github.com/QuantEcon/lecture-python-programming.myst/pull/176

The preview is available here: https://6228365c7ce7b477e5575a73--epic-agnesi-957267.netlify.app/intro.html

@AakashGfude would you mind to take a quick look and let me know what you think.

executablebooks / sphinx-exercise

ENH: Add gated directives to support code-cells for exercises and solutions #45

Basic Syntax

Codecov Report