search

guzzle / guzzle_sphinx_theme

Sphinx theme used by Guzzle
MIT License
170 stars 60 forks source link

Respect the html_use_smartypants option #24

Open tbekolay opened 7 years ago

tbekolay commented 7 years ago

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 commented 7 years ago

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

tbekolay commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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!

mtdowling commented 7 years ago

Thanks, @tbekolay! This looks great!

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

kyleknap commented 7 years ago

@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: https://github.com/arximboldi/immer/commit/4b82bbbfac00da552a0d5edfdc349e0d35b6f699, 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 commented 7 years ago

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