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

Add experimental HTML5 writer #3464

Merged

Conversation

shibukawa
Copy link
Contributor

Subject:

Feature or Bugfix

  • Feature

Purpose

  • Provides HTML5 builder
  • Creates EPUB file that passes EPUB validator.

Detail

  • if html_experimental_html5_builder option is specified, use docutils' html5_polyglot Writer instead of html4css1 Writer.

Relates

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

It looks great! LGTM with nits

doc/conf.py Outdated
@@ -5,6 +5,7 @@
import re
import sphinx

html_experimental_html5_builder = True
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this close to html_* entries?

doc/config.rst Outdated

Output is processed with HTML5 writer. This feature needs docutils 0.13.1 or newer. Default is ``False``.

.. versionadded:: 1.5
Copy link
Member

Choose a reason for hiding this comment

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

This will be released as 1.6.

doc/config.rst Outdated
@@ -1088,6 +1088,11 @@ that use Sphinx's HTMLWriter class.

Output file base name for HTML help builder. Default is ``'pydoc'``.

.. confval:: html_experimental_html5_builder

Output is processed with HTML5 writer. This feature needs docutils 0.13.1 or newer. Default is ``False``.
Copy link
Member

Choose a reason for hiding this comment

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

The name and descriptions are different. which is correct; html5_builder or HTML5 Writer?

@@ -1320,6 +1334,7 @@ def setup(app):
app.add_config_value('html_search_options', {}, 'html')
app.add_config_value('html_search_scorer', '', None)
app.add_config_value('html_scaled_image_link', True, 'html')
app.add_config_value('html_experimental_html5_builder', False, 'html')
Copy link
Member

Choose a reason for hiding this comment

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

We need to warn if users enable HTML5 builder without docutils-0.13.


def html5_writer_available():
import docutils
return list(map(int, docutils.__version__.split('.'))) > [0, 13, 0]
Copy link
Member

Choose a reason for hiding this comment

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

You can use sphinx.util.docutils.__version_info__ instead.

@@ -612,3 +612,8 @@ def status_iterator(iterable, summary, color="darkgreen", length=0, verbosity=0,
yield item
if l > 0:
logger.info('')


def html5_writer_available():
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd like to name the functions which return a boolean value to is_*(). What do you think?

In addition, please add type annotation.
For this function, it's better to annotate like following:

def html5_writer_available():
    # type: () -> bool

@@ -0,0 +1,934 @@
# -*- coding: utf-8 -*-
"""
sphinx.writers.html5
Copy link
Member

Choose a reason for hiding this comment

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

Could you tell me how did you make this? This writer is too large to review whole of code. So I'd like to know the difference with html4 writer.
As a quick look, it is almost same as html4 writer. So I guess you were copied the html4 writer and rewrote the parent class. is this right?

@tk0miya
Copy link
Member

tk0miya commented Feb 26, 2017

It seems some testcases are broken. Could you check them please?

BTW, it seems there are no testcases for HTML5 writer.
At least, we have to invoke it once.

@tk0miya tk0miya added type:enhancement enhance or introduce a new feature builder:html labels Feb 26, 2017
@tk0miya tk0miya added this to the 1.6 milestone Feb 26, 2017
@shibukawa shibukawa force-pushed the feature/experimental_html5_writer branch 3 times, most recently from 4980d3a to 25c5e3c Compare March 1, 2017 01:56
@shibukawa
Copy link
Contributor Author

shibukawa commented Mar 1, 2017

This is my code memo:

    This class is an almost copy of HTMLTranslator because:

    - docutils still uses old style class for Python 2.x. In this case,
      HTMLTranslator and HTML5Translator share logic with different parent
      class. But mixing old and new styles with multiple inheritance
      would bring potential difficult issue.
    - In future (Sphinx 2000?), Sphinx would drop HTML4 support.
      At that time, dropping task becomes easy if they are independent.

    Current different points with ``sphinx.writers.html`` are:

    - HTMLWriter is completely same. So this file omit it.
    - ``visit_table()`` adds ``docutils`` class to ``<table>`` tag
      like HTML4CSS1 writer of docutils.
      But in future it should be removed (now I keep it to keep
      compatibility with existing themes.
    - Omit quick fix to remove named HTML entity (#3452).
      ``visit_option_group()``, ``visit_entry()``, ``visit_field_name()``
      are removed.

    HTML5 results have changes from HTML4 ones:

    - ``admonition`` nodes doesn't add ``first`` and ``last`` classes
      to their children. CSS3 pseudo-classes are enought to replace
      them in theme.
    - Many ``<p>`` nodes (for example in ``<li>`` tags)
    - Doc field and field list doesn't use ``<table>``, but ``<dt>``
      and ``<dd>``.

@shibukawa shibukawa force-pushed the feature/experimental_html5_writer branch from 25c5e3c to 780f280 Compare March 1, 2017 01:59
@@ -625,6 +626,8 @@ def add_node(self, node, **kwds):
translator = self._translators.get(key)
if translator is not None:
pass
elif key == 'html' and is_html5_writer_available():
Copy link
Member

Choose a reason for hiding this comment

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

html_experimental_html5_writer is not referred here.
If I remember correctly, the config object is instantiated until this phase. so we should refer it to determine which writer is used.

CHANGES Outdated
@@ -74,6 +74,8 @@ Features added
* Make ``'extraclassoptions'`` key of ``latex_elements`` public (refs #3480)
* #3463: Add warning messages for required EPUB3 metadata. Add default value to
``epub_description`` to avoid warning like other settings.
* HTML buildre uses experimental HTML5 writer if ``html_experimental_html5_builder`` is True
and docutils 0.13.1 and newer is installed.
Copy link
Member

Choose a reason for hiding this comment

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

docutils-0.13.1 is a bug fix release. so this would work with 0.13.

doc/config.rst Outdated
@@ -1088,6 +1088,11 @@ that use Sphinx's HTMLWriter class.

Output file base name for HTML help builder. Default is ``'pydoc'``.

.. confval:: html_experimental_html5_writer

Output is processed with HTML5 writer. This feature needs docutils 0.13.1 or newer. Default is ``False``.
Copy link
Member

Choose a reason for hiding this comment

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

This should also be docutils-0.13.

doc/config.rst Outdated

Output is processed with HTML5 writer. This feature needs docutils 0.13.1 or newer. Default is ``False``.

.. versionadded:: 1.5
Copy link
Member

Choose a reason for hiding this comment

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

It is released as 1.6. Please update this.

self.app.warn(' '.join((
'html_experimental_html5_writer is set, but current version is old.',
'Docutils\' version should be or newer than 0.13.1, but %s.',
)) % __version_info__)
Copy link
Member

Choose a reason for hiding this comment

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

Could you check is this work? __version_info__ is a tuple containing version info (cf. (0, 13, 1)).
So python formatting might cause an error.


from sphinx import __display_version__
from util import remove_unicode_literals, strip_escseq
from etree13 import ElementTree
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, etree13 is removed at #3483. Could you merge the HEAD of master (I just merged the change into master now)?

from etree13 import ElementTree
from html5lib import getTreeBuilder, HTMLParser
import pytest
from test_build_html import flat_dict, tail_check, check_xpath
Copy link
Member

Choose a reason for hiding this comment

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

Now import statement is ordered by following:

  1. python standard libraries
  2. 3rd party libraries (pytest, six and so on)
  3. sphinx-core
  4. files under tests/

Could you reorder them please?


Experimental docutils writers for HTML5 handling Sphinx' custom nodes.

This class is an almost copy of HTMLTranslator because:
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment needed? Certainly, I requested the difference of writers. But it is only used for reviewing.
I worried about this comment will not be maintained in future version.

This comment describes differences with HTML4 writer mainly. But the old writer will be deprecated in nearly future. I think it is not a description of this codes, and it is used temporarily.
Therefore it would be better to use github-comments or other temporarily space, not in source code.

@shibukawa shibukawa force-pushed the feature/experimental_html5_writer branch from 780f280 to da5996e Compare March 1, 2017 17:04
@shibukawa
Copy link
Contributor Author

updated

@shibukawa shibukawa force-pushed the feature/experimental_html5_writer branch from da5996e to 7aa22b3 Compare March 1, 2017 17:12
@shibukawa shibukawa force-pushed the feature/experimental_html5_writer branch from 7aa22b3 to 0ef9ac5 Compare March 2, 2017 00:57
@tk0miya tk0miya merged commit 7332023 into sphinx-doc:master Mar 2, 2017
@tk0miya
Copy link
Member

tk0miya commented Mar 2, 2017

Merged, Thank you for great work.
To be HTML5 writer official, we should confirm the behavior and improve it step by step.
I can't say the roadmap of that now, but I'd like to be it official at 1.7 release.

Anyway, thank you always. I'm very appreciate you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
builder:html type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants