Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exceptions in separate section #170

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sizmailov
Copy link
Contributor

This PR splits Classes section in two: Classes and Exceptions.

@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #170 into master will increase coverage by 1.74%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #170      +/-   ##
===========================================
+ Coverage   98.25%   100.00%   +1.74%     
===========================================
  Files          27         1      -26     
  Lines        6647       353    -6294     
  Branches       44        44              
===========================================
- Hits         6531       353    -6178     
+ Misses        116         0     -116     
Impacted Files Coverage Δ
plugins/m/components.py
plugins/dot2svg.py
plugins/m/link.py
plugins/m/qr.py
plugins/m/gl.py
plugins/m/alias.py
plugins/m/sphinx.py
plugins/m/plots.py
plugins/ansilexer.py
plugins/latex2svgextra.py
... and 15 more

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 e6eff54...7778fed. Read the comment docs.

@@ -139,6 +142,16 @@ <h2><a href="#data">Data</a></h2>
</dl>
</section>
{% endif %}
{% if page.exceptions %}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a separate variable for exceptions. The alternatives of has_exceptions/has_classes variables or manual checks in templates look worse to me.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually keep them together in page.classes and branch in the template. That way people have more flexibility, either have them separate or put them sorted together with classes into a single "Classes" section in a modified template if they want.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, except that I'd put exceptions together with classes into page.classes and then let people branch in the template (or not).

Assuming you wouldn't have time to get back to this project in the near future, I'll make the change when I get to merging this.

@@ -139,6 +142,16 @@ <h2><a href="#data">Data</a></h2>
</dl>
</section>
{% endif %}
{% if page.exceptions %}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually keep them together in page.classes and branch in the template. That way people have more flexibility, either have them separate or put them sorted together with classes into a single "Classes" section in a modified template if they want.

@mosra
Copy link
Owner

mosra commented Jan 6, 2022

Actually I don't know what to do with this. Attempted to merge this with the above-suggested modification, but halfway through I realized the distinction is done in module/class pages, but search shows both exceptions and classes as still classes, and then I'm not really sure if such distinction is actually that much important to be hardcoded this way, or whether this whole thing shouldn't be completely optional. And then there's dataclasses and generators and slots and shouldn't these also have a distinct treatment? Where to draw the line?

I get that right now the tool doesn't properly parse inheritance hierarchies and as such it's rather hard to see what's an exception and what not. However adding an is_exception property feels more like a hack to me than a real solution.

@mosra mosra added the needs fermenting Not sure what to do with this yet, give it time. label Jan 6, 2022
@sizmailov
Copy link
Contributor Author

I don't have an answer for that. I did a minimal change that worked for my use case. I wanted separation of exceptions because of their lesser importance than regular classes in my specific library.
But you are right, it is not an universal pattern, someone might desire another grouping scheme...

It feels like moving branching to templates is the right direction. I think some new convenient properties/functions would help to avoid template code repetition and simplify it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs fermenting Not sure what to do with this yet, give it time.
Development

Successfully merging this pull request may close these issues.

2 participants