executablebooks / sphinx-exercise

A Sphinx extension for producing exercise and solution directives.
https://ebp-sphinx-exercise.readthedocs.io
MIT License
18 stars 6 forks source link

πŸ‘Œ IMPROVE: Bug fix, latex support and some refactoring #33

Closed AakashGfude closed 2 years ago

AakashGfude commented 2 years ago
welcome[bot] commented 2 years ago

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:

mmcky commented 2 years ago

thanks @AakashGfude let me know when you're done with this refactor etc. Then I can dive in and continue work on the gated directive idea to support code execution to propose to the EBP community.

codecov-commenter commented 2 years ago

Codecov Report

Merging #33 (ac348e7) into master (78bf69b) will decrease coverage by 7.59%. The diff coverage is 85.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   96.30%   88.71%   -7.60%     
==========================================
  Files           3        4       +1     
  Lines         325      381      +56     
==========================================
+ Hits          313      338      +25     
- Misses         12       43      +31     
Flag Coverage Ξ”
pytests 88.71% <85.82%> (-7.60%) :arrow_down:

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

Impacted Files Coverage Ξ”
sphinx_exercise/utils.py 65.62% <65.62%> (ΓΈ)
sphinx_exercise/nodes.py 74.15% <74.15%> (ΓΈ)
sphinx_exercise/__init__.py 95.55% <96.69%> (+0.62%) :arrow_up:
sphinx_exercise/directive.py 98.75% <100.00%> (+0.03%) :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 78bf69b...ac348e7. Read the comment docs.

AakashGfude commented 2 years ago

@mmcky This should be ready to go.

AakashGfude commented 2 years ago

@AakashGfude just going through these changes. I see a lot of changes to object names to make them less general. I think this makes the code a lot more readable πŸ‘ but is there any tradeoff here?

Are these objects becoming less general and code becoming more duplicated or is there very little overlap.

An approach of less general was done, to prevent overwriting/interference from other extensions. For example, node names like enumerable_node, unenumerable_node were getting affected by other extensions. Have tried to reduce duplicity. Let me know if there's any particular instance that is catching your attention.

mmcky commented 2 years ago

@AakashGfude just going through these changes. I see a lot of changes to object names to make them less general. I think this makes the code a lot more readable πŸ‘ but is there any tradeoff here? Are these objects becoming less general and code becoming more duplicated or is there very little overlap.

An approach of less general was done, to prevent overwriting/interference from other extensions. For example, node names like enumerable_node, unenumerable_node were getting affected by other extensions. Have tried to reduce duplicity. Let me know if there's any particular instance that is catching your attention.

thanks @AakashGfude mainly curious. They aren't large methods so looks good to me. After adding the documentation / comments -- please feel free to merge.

AakashGfude commented 2 years ago

thanks @AakashGfude mainly curious. They aren't large methods so looks good to me. After adding the documentation / comments -- please feel free to merge.

I will just merge this one now? So that you can experiment with that prototype. Will reiterate tomorrow, to add comments and any other improvements.

welcome[bot] commented 2 years ago

Congrats on your first merged pull request in this project! :tada: congrats
Thank you for contributing, we are very proud of you! :heart: