Factor out common logic from HTMLTranslator and HTML5Translator #10250
Conversation
jbms
force-pushed
the
deduplicate-html-and-html5
branch
from
835e8d3
to
49a77ad
Compare
Previously, HTML5Translator contained a lot of code copied from HTMLTranslator, which created a maintenance burden and made it difficult to know what the actual differences are. With this change the common logic is factored out into a mixin, which eliminates the code duplication. This change also elimintes the `generate_targets_for_listing` fix from HTMLTranslator now that Sphinx depends on a version of docutils that contains the fix.
jbms
force-pushed
the
deduplicate-html-and-html5
branch
from
49a77ad
to
c4b059f
Compare
|
-1: I feel this makes HTML writers strongly coupled. Then it makes removing the HTML4 writer from core difficult. It's better to discuss the deprecation of the HTML4 writer to keep Sphinx core simple. |
|
It might not be clear from the diffs, but after applying this refactor you can see that the remaining differences between the two writers are very minor, so I don't think duplicating the code is warranted. I noticed that a lot of changes that have been applied since the html5 writer was copy and pasted from the original one have needed to be applied to both. The reason I made this change is that, the change to inline code highlighting also would have needed to be duplicated in both writers. |
|
I'm also -1 on this -- for the reasons @tk0miya listed and also as I find mixin classes generally harder to reason about. A |
|
I really think having that much duplication is untenable --- if you notice almost every change since html5.py was created by copying html.py has had to be duplicated to both files. I personally only used the html5 writer, but it looks like epub and some of the other help formats depend on the html4 writer, so even if you decide you don't care about very old browsers, it seems like the html4 writer will still need to stay around ~forever. So I don't think we can think of it as just a temporary migration issue. An alternative way to avoid the duplication would be to have a single class that handles both html and html5, using conditional logic to handle the differences. Since we need to inherit from different docutils writers this class would have to be defined in a function, but I think it can be made to work. Would that be preferable? |
Feature or Bugfix
Purpose
Previously, HTML5Translator contained a lot of code copied from HTMLTranslator,
which created a maintenance burden and made it difficult to know what
the actual differences are.
With this change the common logic is factored out into a mixin, which
eliminates the code duplication.
This change also elimintes the
generate_targets_for_listingfix fromHTMLTranslator now that Sphinx depends on a version of docutils that
contains the fix.