MAINT: review and refactor package #37

mmcky · 2021-10-28T06:04:18Z

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

It introduces:

separate exercise and solution directive code (rather than inheriting from the same base) which lowers cognitive load when working with the objects.
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.
moves post_transforms into post_transforms.py
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
[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.
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 · 2021-10-28T06:04:20Z

Thanks for submitting your first pull request! You are awesome! 🤗

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! 🎉

codecov-commenter · 2021-10-28T06:06:21Z

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%)`	⬇️

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%)`	⬇️
sphinx_exercise/__init__.py	`94.28% <85.71%> (-1.37%)`	⬇️
sphinx_exercise/nodes.py	`89.53% <85.71%> (-8.09%)`	⬇️
sphinx_exercise/post_transforms.py	`94.96% <94.96%> (ø)`
sphinx_exercise/directive.py	`98.31% <97.80%> (-0.42%)`	⬇️
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.

… reference support

mmcky · 2021-11-16T05:36:24Z

mmcky · 2021-11-18T04:34:08Z

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

mmcky · 2021-11-19T00:13:01Z

@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 · 2021-11-19T03:16:21Z

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

mmcky · 2021-11-19T03:52:00Z

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 · 2021-11-19T19:18:03Z

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

mmcky · 2021-11-21T23:58:13Z

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.

mmcky · 2021-11-23T22:29:11Z

tests/test_exercise/_enum_numref_mathtitle.html

 <p>This is a third reference <a class="reference internal" href="_enum_mathtitle_label.html#test-exc-label-math"><span class="std std-numref">some text 1</span></a>.</p>
-<p>This is a fourth reference <a class="reference internal" href="_enum_mathtitle_label.html#test-exc-label-math"><span class="std std-numref">some text 1</span></a>.</p>
-<p>This is a fifth reference <a class="reference internal" href="_enum_mathtitle_label.html#test-exc-label-math"><span>some text Test <span class="math notranslate nohighlight">\(\mathbb{R}\)</span></span></a>.</p>
+<p>This is a fourth reference <a class="reference internal" href="_enum_mathtitle_label.html#test-exc-label-math"><span class="std std-numref">some text Exercise</span></a>.</p>


@AakashGfude this is one of the numref fixtures I am referring to

mmcky · 2021-11-24T04:19:33Z

sphinx_exercise/post_transforms.py

+    title = node.children[0]
+    if isinstance(title, exercise_title):
+        node_number = get_node_number(app, node, "exercise")
+        updated_title = exercise_title()


@AakashGfude perhaps this new exercise_title node is detached from the parent?

@mmcky it does get attached to the admonition node later here:

sphinx-exercise/sphinx_exercise/post_transforms.py

Line 34 in 84a3d2a

node.children[0] = updated_title

it replaces the child for sure -- but that isn't necessarily the same as

node += updated_title

I'll take a quick look

@mmcky adding visit, depart exercise_title support for latex here:

sphinx-exercise/sphinx_exercise/__init__.py

Line 179 in 84a3d2a

app.add_node(exercise_title, html=(visit_exercise_title, depart_exercise_title))

, gets rid of the
encountered title node not in section, topic, table, ' 'admonition or sidebar
error, but needs some formatting for proper latex output.

Yeah it get's detached as the node metadata isn't retained 😮‍💨

> sphinx-exercise/.tox/py38/lib/python3.8/site-packages/sphinx_exercise/post_transforms.py(25)resolve_enumerated_exercise_node_title() -> title = node.children[0] (Pdb) node.children[0] <exercise_title: <#text: 'Exercise'><exercise_subtitle...>> (Pdb) node.children[0].parent <exercise_enumerable_node "exercise-1": <exercise_title...><section...>> (Pdb) c >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB continue (IO-capturing resumed) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB set_trace (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > sphinx-exercise/.tox/py38/lib/python3.8/site-packages/sphinx_exercise/post_transforms.py(39)resolve_enumerated_exercise_node_title() -> return node (Pdb) node.children[0].parent

should be fixed by ebf67f5

Thanks a lot @mmcky

mmcky · 2021-11-24T04:19:49Z

sphinx_exercise/post_transforms.py

+    exercise_target = app.env.sphinx_exercise_registry[node.get("target_label")]["node"]
+    title = node.children[0]
+    if isinstance(title, solution_title):
+        new_title = solution_title()


@AakashGfude similarly here?

similarly for this one.

adding visit, depart exercise_title support for latex here:

@AakashGfude indeed. All the LaTex elements are current raise NotImplementedError for title and subtitle visit/depart.

…le for solution nodes

mmcky · 2021-11-29T03:56:27Z

@AakashGfude I think this is looking pretty good.

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

QuantEcon/continuous_time_mcs#102

mmcky · 2021-11-30T01:25:19Z

fix LaTeX resolution of \hyperref for enumerated exercise nodes
fix LaTeX resolution of \hyperref for solution nodes

mmcky · 2021-11-30T02:05:04Z

tidy up the code and docs

mmcky · 2021-11-30T02:05:54Z

@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 · 2021-11-30T03:26:00Z

@mmcky Looking good to me.

mmcky · 2021-11-30T21:02:04Z

@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 · 2021-12-01T01:11:50Z

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

choldgraf

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

choldgraf · 2021-12-03T21:25:30Z

sphinx_exercise/post_transforms.py

+                    node.children[0] = inline
+
+                    # TODO: Is it possible for the target_node not to be resolved?
+                    # if not target_node.resolved_title:


is this "if" statement that's commented out intentionally left in? When will it be un-commented?

choldgraf · 2021-12-03T21:26:29Z

sphinx_exercise/post_transforms.py

+
+def resolve_solution_title(app, node, exercise_node):
+    """
+    Resolve Solution Nodes


Can you use slightly more descriptive docstrings here and in general? If somebody had never seen this codebase before, they should know:

What does this function/class do at a high level

What are the outputs and inputs

Any gotchas or extra information that can help them understand what's going on

These don't need to be as well documented as more user-facing functions but I think it's helpful to at least have this minimal info in there

choldgraf · 2021-12-03T21:28:54Z

sphinx_exercise/post_transforms.py

+logger = logging.getLogger(__name__)
+
+
+def build_reference_node(app, target_node):


missing docstring - please add docstrings to all functions

choldgraf · 2021-12-03T21:29:52Z

sphinx_exercise/nodes.py

+# Test Node Functions
+
+
+def is_exercise_node(node):


these functions are straightforward enough they don't need a docstring IMO

choldgraf · 2021-12-03T21:30:13Z

sphinx_exercise/nodes.py

@@ -2,149 +2,171 @@
 sphinx_exercise.nodes
 ~~~~~~~~~~~~~~~~~~~~~

-Enumerable and unenumerable nodes
+Sphinx Exercise Nodes

 :copyright: Copyright 2020 by the QuantEcon team, see AUTHORS


just a note that in general we've been using "Executable Books Project" for copyright

sure -- just didn't review that when we transferred to EBP :-). 👍

mmcky · 2021-12-05T21:44:53Z

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 · 2021-12-06T03:59:17Z

fixed issue with solution nodes to being clickable to solution nodes

mmcky · 2021-12-06T04:00:40Z

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

MAINT: rework directives into exercise and solution directives

4914e33

mmcky added 7 commits October 29, 2021 10:44

simplify and reduce repeated code

5ac852e

split post_transforms and review

4ddd56f

introduce LaTeX markup object

f176cb7

reorg nodes.py

dc66d0d

remove functions not required

ca24235

odds and end updates commit before references refactor

cc835a7

REF: rework reference post-transforms, in work

a3338bf

mmcky changed the title ~~MAINT: rework directives into exercise and solution directives~~ MAINT: review and refactor package Nov 1, 2021

mmcky added 2 commits November 2, 2021 10:39

Merge branch 'master' into refactor-oct2021

6fbdeff

TST: Updating test solutions for test_exercise

8e70340

mmcky self-assigned this Nov 3, 2021

REFACTOR: update exercise nodes, directives, and post_transforms with…

5b91146

… reference support

mmcky added 2 commits November 18, 2021 15:28

rework references resolution

945a322

rework references resolution

1eaa9b8

update fixtures for final review

962de98

mmcky requested a review from AakashGfude November 19, 2021 00:13

fixes for updating references

84a3d2a

mmcky commented Nov 23, 2021

View reviewed changes

mmcky commented Nov 24, 2021

View reviewed changes

mmcky added 4 commits November 26, 2021 11:24

Reorganise tests

62a94b2

style update for custom titles, ensure mathjax loading if math in tit…

16d4ae0

…le for solution nodes

adding in empty title for enumerated exercise cells for LaTeX

8d0d969

resolve exercise titles for enumerated nodes when targeting latex

1daa7a7

Fix LaTeX issue with reference resolution

20b26d9

[LaTeX] fix reference resolution for solutions

3b9635f

tidy up code and remove unecessary methods

b6520fe

mmcky added 2 commits December 1, 2021 07:37

docs review

cd044bb

minor update to docs

a41e286

AakashGfude approved these changes Dec 1, 2021

View reviewed changes

removing unused functions and unused latex handling

fc158cb

choldgraf approved these changes Dec 3, 2021

View reviewed changes

mmcky added 4 commits December 6, 2021 09:34

final code review

5664626

update docs with developer notes

f6907ab

change to EBP developers

e3686be

additional final edits of docstrings

94fa543

mmcky mentioned this pull request Dec 6, 2021

[ENH] Patch translations #42

Draft

FIX: fix solution node titles are links to exercise node

02d8eff

mmcky merged commit 08c469e into master Dec 6, 2021

mmcky deleted the refactor-oct2021 branch December 6, 2021 23:05

		logger = logging.getLogger(__name__)


		def build_reference_node(app, target_node):

MAINT: review and refactor package #37

MAINT: review and refactor package #37

Conversation

mmcky commented Oct 28, 2021 • edited Loading

welcome bot commented Oct 28, 2021

codecov-commenter commented Oct 28, 2021 • edited Loading

Codecov Report

mmcky commented Nov 16, 2021 • edited Loading

mmcky commented Nov 18, 2021

mmcky commented Nov 19, 2021 • edited Loading

AakashGfude commented Nov 19, 2021

mmcky commented Nov 19, 2021 • edited Loading

choldgraf commented Nov 19, 2021

mmcky commented Nov 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmcky commented Nov 29, 2021

mmcky commented Nov 30, 2021 • edited Loading

mmcky commented Nov 30, 2021 • edited Loading

mmcky commented Nov 30, 2021

AakashGfude commented Nov 30, 2021

mmcky commented Nov 30, 2021

AakashGfude commented Dec 1, 2021

choldgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmcky commented Dec 5, 2021

mmcky commented Dec 6, 2021

mmcky commented Dec 6, 2021

mmcky commented Oct 28, 2021 •

edited

Loading

codecov-commenter commented Oct 28, 2021 •

edited

Loading

mmcky commented Nov 16, 2021 •

edited

Loading

mmcky commented Nov 19, 2021 •

edited

Loading

mmcky commented Nov 19, 2021 •

edited

Loading

mmcky commented Nov 30, 2021 •

edited

Loading

mmcky commented Nov 30, 2021 •

edited

Loading