MAINT: review and refactor package

[mmcky]

This is a refactor PR to make this extension more maintainable.

It introduces:

1. separate exercise and solution directive code (rather than inheriting from the same base) which lowers cognitive load when working with the objects.
2. moves env.exercise_list to env.sphinx_exercise_registry as it was confusing to have exercise and solution nodes going into an object called exercise_list.
3. moves post_transforms into post_transforms.py
4. redesigns the directives and post_transforms by introducing exercise- and solution- titles and subtitles which enables targeted processes within the node (rather than have custom string modification code as before). This integrates more closely with sphinx and let's sphinx translators do the translation work from the ast
5. [latex] resolving hyperref links for exercise admonitions wasn't trivial as it requires recasting numbered references in a post_transform to a custom node to enable processing at the translator layer, as it wasn't possible to integrate with the base sphinx latex translator to include custom title text.
6. this PR introduces a change to how custom title text is formatted in admonition titles and links to admonitions

Exercises:

Solutions:

Links to Solutions:

[welcome[bot]]

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.
Welcome to the EBP community! :tada:

[codecov-commenter]

Codecov Report

Merging #37 (02d8eff) into master (a3cb4f6) will decrease coverage by 2.48%. The diff coverage is 93.27%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
- Coverage   95.83%   93.34%   -2.49%     
==========================================
  Files           4        6       +2     
  Lines         384      436      +52     
==========================================
+ Hits          368      407      +39     
- Misses         16       29      +13     

Flag	Coverage Δ
pytests	93.34% <93.27%> (-2.49%)	:arrow_down:

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

Impacted Files	Coverage Δ
sphinx_exercise/utils.py	56.25% <ø> (-30.24%)	:arrow_down:
sphinx_exercise/__init__.py	94.28% <85.71%> (-1.37%)	:arrow_down:
sphinx_exercise/nodes.py	89.53% <85.71%> (-8.09%)	:arrow_down:
sphinx_exercise/post_transforms.py	94.96% <94.96%> (ø)
sphinx_exercise/directive.py	98.31% <97.80%> (-0.42%)	:arrow_down:
sphinx_exercise/latex.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 a3cb4f6...02d8eff. Read the comment docs.

[mmcky]

Fix the following test collections

• [x] test_exercise
• [x] test_build
• [x] test_solution
• [x] test_latex
• [x] test_reference

[mmcky]

• [ ] review docs
• [ ] review documentation (within code)
• [ ] review fixture diffs to ensure references are correct (as intended)

[mmcky]

@AakashGfude this is getting close. Integrating with the sphinx referencing systems has been a pain and it makes we wonder if we are doing this right. But I think there are not many better alternatives giving xref are resolved in post_transforms and they get applied at the document level rather than the project level.

When you have time would you mind reviewing the fixtures and let me know what you think. The titles have changed a bit to add support for custom title text.

The main changes are I have worked directly with the node rather than carry around metadata etc. The exercise and solution nodes now contain exercise-title and exercise-subtitle nodes to enable identification etc. Anwyay open to thoughts and feedback on the code changes.

If you could also take a look at latex -- I think latex support should now only be needed in the node visit and depart methods etc. I will look at latex next week if you don't have time.

[AakashGfude]

Pretty hefty PR 😬. Will carefully inspect and also add the changes you asked for.

[mmcky]

thanks @AakashGfude I should add that full numfig support isn't 100% but that is the way numfig is implemented so not sure what to do about that (other than take control and write numfig support into the extension for custom titling etc.

I will also add some docs etc. but would value some initial feedback.

[choldgraf]

@mmcky are there particular questions that you have, where you'd like people to focus their reviews?

[mmcky]

thanks @choldgraf I was planning to get @AakashGfude to review (as a first round) then I will apply some final lessons learnt and update the docs. I was then planning to open up for wider review at that stage with some specific questions around integration with references in sphinx.

[choldgraf]

@mmcky feel free to request a review once it's ready for others to look at it!

[mmcky]

@AakashGfude I did another review of the post_transforms for title and reference adjustments and simplified again.

https://github.com/executablebooks/sphinx-exercise/pull/37/commits/1665d17021bb7bd3ca692d857d6bd89485cf3c77

It would be great if you could have a look at the fixtures and let me know what you think (ie. they are resolving to good solutions in html and xml. The latex build is broken as a result of these changes. Let me know what you think of these changes. Cheers.

[mmcky]

@AakashGfude I think this is looking pretty good.

I have opened a test build here on a real-world test case

https://github.com/QuantEcon/continuous_time_mcs/pull/102

[mmcky]

• [x] fix LaTeX resolution of \hyperref for enumerated exercise nodes
• [x] fix LaTeX resolution of \hyperref for solution nodes

[mmcky]

• [x] tidy up the code and docs

[mmcky]

@AakashGfude I think we have now resolved the integration with sphinx references using ref and numref where we can. Let me know what you think of the changes now.

[AakashGfude]

@mmcky Looking good to me.

[mmcky]

@chrisjsewell @choldgraf we have given our best shot at integrating closely with sphinx in this PR. The focus of the re-write has been to alter the nodes in the sphinx.ast to remove custom code that was modifying strings, and using ref and numref when resolving cross-references between exercise, solutions, and references using post_transforms. We need to use post_transforms mainly as pending_xref nodes don't get updated until the post_transform stage with priority 10 -- what we also learnt is that numref is largely resolved at the translator layer (!)

We choose to integrate with the std domain as it allowed the use of ref and numref but that ended up being a lot of work to figure out how that cross-referencing infrastructure worked. I'd welcome any comments you have on this from your experience. I think what we have ended up with is a decent solution. There was one workaround for numbered_references for the latex builders due to a limitation in the default sphinx translator for latex. So we opted to use a post_transform to migrate numbered_reference to exercise_latex_numbered_reference nodes and add in our own simpler visit and depart method. I think this is a good solution pattern when the default just won't work.

[AakashGfude]

@mmcky I think the pop_index is not required anymore https://github.com/executablebooks/sphinx-exercise/pull/37/files#diff-77185f1ed5e85402b80a05b1e8610ab9f33a9188e54393f8bdb3868594d0b5deR99

[mmcky]

I took a quick pass through, with a focus on the docs since you mentioned that the main goal here was maintainability. I think in general it looks good to go (though didn't have time to do a deep dive since this changes a ton of things). The main comment I have is to make sure that the docstrings and documented well-enough that a person who isn't familiar with the codebase could orient themselves relatively quickly. There are likely a few places where there's assumptions being made and missing information because you all know a lot more about how the package works already :-) it might also be helpful to have a short explainer of the structure of this repository, so that others know how it is implemented, where to look for things in the codebase etc. I don't think any of that needs to block this PR, but there could be some low-hanging fruit there if you want to give it a shot

thanks for the review @choldgraf will do a final pass on docs and internal comments + docstrings today. I'l be adding a section with lessons learnt on the design of the package (which can serve as a source for a more general set of notes about writing a sphinx package for EBP)

[mmcky]

• [x] fixed issue with solution nodes to being clickable to solution nodes

[mmcky]

thanks @choldgraf and @AakashGfude I think this is good to be merged now. I'll merge tomorrow morning unless any further comments.