✨ NEW: Add solution directive

[najuzilu]

*** IN PROGRESS ***

This PR introduces a new solution directive. The solution directive will reference an exercise directive. The syntax is as follows:

```{proof:exercise} Exercise 1
:label: ex1-label

This is an exercise directive.
```

```{proof:solution} ex1-label
This is the solution associated with the above exercise.
```

For more information on how this directive behaves, either check the deployed Netlify link below or read through #18.

Additional algorithm and proof tests have been introduced to take into account any new changes introduced with the new directive. These tests include:

• _algo_title_with_inline_math
• _algo_with_labeled_math
• _algo_unlabeled_math
• _proof_inline_math_argument

Solution tests include:

• solution with an enumerable exercise
• solution with an unenumerable + simple title exercise
• solution with an unenumerable+ inline-math title exercise
• solution with an unenumerable + not titled exercise
• solution missing its argument
• solution where the argument is incorrect exercise label
• solution using label and class options
• solution referencing an enumerable exercise
• solution referencing an unenumerable + simple title exercise
• solution referencing an unenumerable + inline-math title exercise
• solution referencing an unenumerable + not titled exercise
• solution referencing incorrect solution label
• solution with duplicate label

[najuzilu]

@AakashGfude maybe you'll be able to provide some guidance regarding the warning test for the solution directive. It doesn't look like rootdir method is executed at all during warnings which could explain why warnings(app) returns an empty string (the dst directory isn't removed prior to build...).

The strange thing is that when I run pytest tests/test_solution.py --verbose, all the tests pass. The test_warnings method fails with the overall pytest --verbose. Do you have any suggestions as to how to proceed?

[codecov]

Codecov Report

Merging #32 into master will increase coverage by 0.48%.
The diff coverage is 98.13%.

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   97.09%   97.58%   +0.48%     
==========================================
  Files           6        6              
  Lines         241      372     +131     
==========================================
+ Hits          234      363     +129     
- Misses          7        9       +2     

Flag	Coverage Δ
#pytests	97.58% <98.13%> (+0.48%)	⬆️

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

Impacted Files	Coverage Δ
sphinxcontrib/prettyproof/domain.py	97.91% <96.22%> (-2.09%)	⬇️
sphinxcontrib/prettyproof/directive.py	99.18% <98.07%> (-0.82%)	⬇️
sphinxcontrib/prettyproof/__init__.py	90.69% <100.00%> (-0.22%)	⬇️
sphinxcontrib/prettyproof/nodes.py	100.00% <100.00%> (+2.70%)	⬆️
sphinxcontrib/prettyproof/proof_type.py	100.00% <100.00%> (ø)

---

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 71fd044...12c64f1. Read the comment docs.

[AakashGfude]

Hey @najuzilu , sorry missed this comment. I guess it is the same thing you talked about on slack and is now solved?

[najuzilu]

Yes, when app.build() is initiated in the test_warnings method of test_solution.py, mybook doesn't display any warnings because it has been previously built (during test_algorithm.py). In order for warnings to display, I have to remove the _build directory and then build. I'll likely move this under the rootdir method to initiate a clean build every time for all tests which should minimize errors like this in the future.

[najuzilu]

@mmcky @AakashGfude I plan to rebase before merging haha.

[najuzilu]

The refactored code allows for easier implementation of other directives which behave similarly to solution. I decided to move the implementation to a new LinkedDirective with supporting linked_node object and visit_linked_node, depart_linked_node methods.

[choldgraf]

this is really cool! One question - how "proof-specific" is this functionality? I ask because question and answer directives are one of the more-requested features in jupyter book, and I wonder if it's worth generalizing this a bit so that you can use it outside of the context of proofs. What do you all think?

[jstac]

@mmcky made the same point.

Of course there's nothing stopping people using the solutions / exercises without using any of the theorem / proof / lemma environments. But I guess your point is the name of the package will confuse people, so perhaps the solutions / exercises should be broken out into something separate? Or should the package itself be renamed?

I agree that the solutions / exercises environments will be popular on their own.

[choldgraf]

yeah I was imagining if this is a generically-useful directive, we could split it out into something like sphinx-exercise (or sphinxcontrib-exercise if @mmcky prefers ;-) ) and then prettyproof could use that package, but it would be more generically useful (and easier for people to discover and use). I think it could increase the adoption of the tool - I can see users wondering "why do I need to install a proof/theorem extension just to get an exercise/solution directive?"

[jstac]

I'm fully in favor of this. @najuzilu has done some really nice work here, worthy of two extensions :-)

[najuzilu]

Ah yes, we plan to move these over to a new repo. I've already started some work on that.

My main goal is to get this to work correctly with the current infrastructure. It will be very easy to migrate it to sphinx-exercise (shorter is better in my opinion). I'll open an issue regarding this once we merge the PR successfully. How does that sound? @choldgraf

[choldgraf]

@najuzilu that works for me - though we should make the switch relatively quickly because I imagine there will be an API deprecation (as presumably people would just use ```{solution} rather than ```{proof:solution}

[mmcky]

thanks @jstac -- @najuzilu let us know if you need any help setting up the new repo. Looking forward to sphinx-exercise. Also given we have just transferred this over to ebp would you like to keep this package as sphinxcontrib or move it to a similar naming convention sphinx- or sphinxext-

[choldgraf]

my preference is for sphinx- but I'm happy with either...I always found the sphinxcontrib thing for independent repos to be a bit weird because it's not part of the sphinxcontrib org, but IMO it's not a huge deal either way.

[mmcky]

my preference is for sphinx- but I'm happy with either...I always found the sphinxcontrib thing for independent repos to be a bit weird because it's not part of the sphinxcontrib org, but IMO it's not a huge deal either way.

👍 let's use sphinx- for sphinx extensions in ebp. I agree and in favour of this. As you say it isn't part of the formal sphinxcontrib community.

[najuzilu]

If everyone's okay with it, I'm going to go ahead and close this PR as I've transferred over the work to sphinx-exercise. I have already made a PR to depreciate immediately the exercise directive which has also been migrated to sphinx-exercise.