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

Respect the html_use_smartypants option #24

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

Conversation

tbekolay
Copy link
Contributor

@tbekolay tbekolay commented Jan 1, 2017

See the html_use_smartypants option docs. The way Sphinx implements this is by using a different HTMLTranslator subclass. This PR respects that config option by subclassing the SmartyPantsHTMLTranslator when the option is True (which is the default).

Note that this PR's branch is based on the set_translator branch from #23, so that one should be reviewed/merged first.

@arximboldi
Copy link

I was just hit by this problem today, thanks @tbekolay! 😄

@tbekolay
Copy link
Contributor Author

I think there might be an easier way to do this using app.add_node rather than subclassing one of the HTML translators... I'll look into this shortly.

@mtdowling
Copy link
Member

Cool thanks. Taking a look at this now, and I'm getting an error when building the demo:

$ make html
sphinx-build -b html -d _build/doctrees   . _build/html
Running Sphinx v1.3.6
making output directory...
A Translator for the html builder is changed.
loading pickled environment... not yet created
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 4 source files that are out of date
updating environment: 4 added, 0 changed, 0 removed
reading sources... [100%] table-with-code                                                                                                                                                                          
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [ 25%] index                                                                                                                                                                                     
Exception occurred:
  File "/Users/dowling/.pyenv/versions/2.7.6/envs/aws-cli/lib/python2.7/site-packages/sphinx/writers/html.py", line 52, in translate
    self.document)
TypeError: object() takes no parameters
The full traceback has been saved in /var/folders/2d/lyk6qb5d4b33v63mvvz_rmnc9pgt8t/T/sphinx-err-nclvLD.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [html] Error 1

$ pip list
[...]
guzzle-sphinx-theme (0.7.11, /Users/dowling/projects/guzzle/guzzle_sphinx_theme)
Sphinx (1.3.6)
[...]

@tbekolay
Copy link
Contributor Author

I've updated this PR; the main commit now uses app.add_node, which makes the code quite a bit simpler. I tested building the demo on lots of Sphinx versions, and it works for me on Sphinx 1.2 and greater, so I updated setup.py to reflect that.

I also added a commit to remove some unused imports; if you want me to remove that commit let me know, or feel free to omit it in the merge!

Previously, this was ignored because the config option is implemented
by using a different HTMLTranslator in the case that the option is
True (which is the default). This PR switches from subclassing the
non-SmartyPants HTMLTranslator and instead uses calls to
`app.add_node` to override the visit/depart functions.

This removes the need to use `app.set_translator`, making our
Sphinx requirement more flexible. I tested building the demo with
the current version of Sphinx and earlier, and it worked all the
way to Sphinx 1.1, which failed for something unrelated to this change.
`setup.py` was updated accordingly.
And fixed some indentation to follow PEP8 style.
@mtdowling
Copy link
Member

Thanks, @tbekolay! This looks great!

@kyleknap does this look good to you (e.g., how does this fare on the aws-cli docs)?

@kyleknap
Copy link
Contributor

kyleknap commented Feb 3, 2017

@mtdowling
This will break us with this error (building the boto3 docs):

$ make html
sphinx-build -b html -d build/doctrees   source build/html
Running Sphinx v1.2.3
loading pickled environment... done

Extension error:
Could not find guzzle_sphinx_theme.HTMLTranslator (needed for html_translator_class setting) (exception: 'module' object has no attribute 'HTMLTranslator')
make: *** [html] Error 1

The reasoning being that if you remove the guzzle_sphinx_theme.HTMLTranslator class it will not be found when you specify it in your conf.py file: https://github.com/boto/boto3/blob/develop/docs/source/conf.py#L103

You can even see that in a related commit: arximboldi/immer@4b82bbb, the html_translator_class had to be removed to get the doc build to work.

If this change goes out in a minor version bump, it won't affect our doc build: https://github.com/boto/boto3/blob/develop/requirements-docs.txt#L2. But it could affect other developer's doc builds if they do not lock to a specific version.

Looking into the sphinx docs, it looks like that option is deprecated: http://www.sphinx-doc.org/en/stable/config.html so I am not sure how many people is using it. But just wanted to give a heads up.

@tbekolay
Copy link
Contributor Author

tbekolay commented Feb 3, 2017

We can introduce this with backwards compatibility if desired (by doing something like HTMLTranslator = sphinx.SmartyPantsHTMLTranslator if html_use_smartypants else sphinx.HTMLTranslator)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants