-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Do not apply smart quotes if Sphinx says no, even if Docutils says yes #4360
Conversation
sphinx/io.py
Outdated
@@ -53,6 +53,8 @@ def __init__(self, app, parsers={}, *args, **kwargs): | |||
parser = parser_class() | |||
if hasattr(parser, 'set_application'): | |||
parser.set_application(app) | |||
if hasattr(parser, 'set_smartquotes_allowed'): | |||
parser.set_smartquotes_allowed(app.env.settings['smart_quotes']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I understand, this will be called from a BuildEnvironment object read_doc()
method, when a reader
is being set up, and at that time the app.env.settings['smart_quotes']
always exists with value either True or False due to earlier code executed by the BuildEnvironment instance.
Now, perhaps the accessor method is superfluous here and we should set the Boolean directly rather than having such an instance method added to RSTParser class.
@tk0miya perhaps we can reconsider now the 79df05b which reverted bfd39c1 ? I forgot the reason why bfd39c1 was reverted. With this PR, a Sphinx edit1: "override" is not correct; it applies only in case the edit2: I have ressurected bfd39c1 in modified form in 2nd commit of this PR. |
I should clarify that initial commit of this PR does only what the title says: if Sphinx setting disallows smart quotes, then they are not applied and Docutils setting is ignored. But if Sphinx setting does allow smart quotes, then they still can be unactivated if Docutils configuration files |
sphinx/parsers.py
Outdated
@@ -58,12 +58,21 @@ def set_application(self, app): | |||
class RSTParser(docutils.parsers.rst.Parser): | |||
"""A reST parser customized for Sphinx.""" | |||
|
|||
smartquotes_allowed = True | |||
|
|||
def set_smartquotes_action(self, action): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make RSTParser
class a subclass of sphinx.parsers.Parser
. After that we can refer config object by self.config
.
I think storing status to class variable is not good way. Because it behaves like global variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, done (will comment after push)
doc/config.rst
Outdated
@@ -353,6 +353,39 @@ General configuration | |||
|
|||
.. versionadded:: 1.3 | |||
|
|||
.. confval:: smart_quotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the basic concept of this PR. But we would need to be able to disable smart_quotes by several condition.
For example, #4142 tells us to disable it for Japanese document. I saw somebody wants to disable it for arbitrary builder in some issues.
So I think this should not be boolean. (but I don't have good alternative idea...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason of the revert is only it is not intended commit. It was not my completed work as I commented above. I think simple boolean setting is not good for this case. So we have to consider the way to configure smart_quotes. It's okay to introduce a configuration for smart_quotes itself to me. |
I'd forggeten to mention about parsers. I think smart quotes is not only for reST files. It is a setting for representation, so it should effect to other formats. |
sphinx/parsers.py
Outdated
# type: (Sphinx) -> None | ||
Parser.set_application(self, app) | ||
if hasattr(SphinxSmartQuotes, 'smartquotes_action'): | ||
SphinxSmartQuotes.smartquotes_action = self.env.settings['smartquotes_action'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a problem here which is that afaik SphinxSmartQuotes is a class, not an instance of that class, and in get_transforms below it is the class object which is added to list of transforms. Hence I needed to modify the class attribute here. Don't know if this can have undesired consequences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, class attribute is a kind of global variable. So modifying it is not good way. So it would be better to update it temporarily like following:
from contextlib import contextmanager
@contextmanager
def sphinx_smartquotes_action(env):
try:
original = SphinxSmartQuotes.smartquotes_action
SphinxSmartQuotes.smart_quotes_action = env.config.smartquotes_action
yield
finally:
SphinxSmartQuotes.smart_quotes_action = original
# in env.doc_read() method:
with sphinx_smartquotes_action(self):
doctree = read_doc(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will do.
sphinx/environment/__init__.py
Outdated
@@ -692,6 +691,7 @@ def read_doc(self, docname, app=None): | |||
break | |||
else: | |||
self.settings['smart_quotes'] = False | |||
self.settings['smartquotes_action'] = self.config.smartquotes_action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docutils has no smartquotes_action user config setting yet; but it introduced the docutils.transforms.universal.SmartQuotes class attribute at 0.14, and it is useful for us, because Sphinx users may want to for example not apply smart quotes to ellipsis conversion (from ...
). For Docutils < 0.14, there was at some point some bad config which educated backticks also, and we override that in our own sphinx.util.smartypants (this module is used by Sphinx only for Docutils < 0.14).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for setting this into self.settings
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in anticipation that Docutils might offer such an option in future to configure the smartquotes_action
attribute of its docutils.transforms.universal.SmartQuotes, which we inherit for SphinxSmartQuotes. Also, I did this before RSTParser inherited from Parser hence got access to app. Thinking twice, it might indeed be better not to set this into self.settings
, else we will again have problem that a Docutils config file would override the Sphinx user conf.py, if such an option is introduced in future by Docutils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If docutils does not implement the option yet, it might be too premature action. -1 for adding key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed it from self.settings
, but I do keep it as confval because it is useful for Sphinx users. Is that ok?
My knowledge of how Sphinx interacts with Docutils remains quite limited. At 69cf328 you created a class |
@tk0miya I have added confval setting I have not addressed the issue about extending PR to other than RSTParser, because my knowledge of Sphinx/Docutils workflow is currently too limited. |
doc/config.rst
Outdated
that in case of invocation of ``make`` with multiple builder names, the | ||
first one counts. Also, when using ``sphinx-build`` in succession for | ||
different builders, one needs to use ``-E`` option (e.g. in case of an | ||
``html`` build followed by a ``man`` build, or conversely.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry, I am not used to sphinx-build
but as it creates by default a .doctrees
individual to each target, there is no need for -E
. The problem arises when using the "make-mode" only. Will fix docs.
sphinx/environment/__init__.py
Outdated
@@ -677,21 +677,37 @@ def read_doc(self, docname, app=None): | |||
language = self.config.language or 'en' | |||
self.settings['language_code'] = language | |||
if 'smart_quotes' not in self.settings: | |||
self.settings['smart_quotes'] = True | |||
self.settings['smart_quotes'] = self.config.smart_quotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that Sphinx core has a smart_quotes
config setting, possibly the logic for
if 'smart_quotes' not in self.settings:
(see #4112) is gone? can an extension override a config setting rather than add it to environment as done at sphinx-contrib/spelling@6209fa8 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not a public interface, but it is needed for workaround. So please keep this as is.
Note: It might be refactored in future. Personally, I feel read_doc
is not a part of responsibility of "BuildEnvironment".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will leave it. I find "BuildEnvironment" complicated...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, BuildEnvironment
has multiple responsibility. So I'd like to split it into some modules in future.
Currently the sphinx-quickstart created Makefile (whether make-mode enabled or not) configures builds to share Then the builders listed in |
doc/config.rst
Outdated
|
||
This is a ``dict`` whose default is:: | ||
|
||
{'language': ['ja'], 'builder': ['man', 'text']} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to rename keys plural form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using singular for easing up re-use of the key as getattr(self.config, key)
, but I agree it is more self-explanatory to use plurals. In theory any config key is usable here to condition usage of Smart Quotes, but in practice I guess I can just drop the final "s". Or we allow only conditioning on language and builder? Also, I will explain better in docs that the logic is simply "OR".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of the usecase for any config key (without language
). So I feel it's okay to restrict the key to languages
and builders
.
Also, I will explain better in docs that the logic is simply "OR".
+1
sphinx/environment/__init__.py
Outdated
@@ -677,21 +677,37 @@ def read_doc(self, docname, app=None): | |||
language = self.config.language or 'en' | |||
self.settings['language_code'] = language | |||
if 'smart_quotes' not in self.settings: | |||
self.settings['smart_quotes'] = True | |||
self.settings['smart_quotes'] = self.config.smart_quotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not a public interface, but it is needed for workaround. So please keep this as is.
Note: It might be refactored in future. Personally, I feel read_doc
is not a part of responsibility of "BuildEnvironment".
sphinx/environment/__init__.py
Outdated
if valname == 'builder': | ||
# this will work only for checking first build target | ||
if self.app.builder.name in vallist: | ||
self.settings['smart_quotes'] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, smart quotes transform should be invoked after the pickling (write phase) to support each builders.
I know it is a bit big refactoring. so +1 this for stable.
sphinx/parsers.py
Outdated
# type: (Sphinx) -> None | ||
Parser.set_application(self, app) | ||
if hasattr(SphinxSmartQuotes, 'smartquotes_action'): | ||
SphinxSmartQuotes.smartquotes_action = self.env.settings['smartquotes_action'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, class attribute is a kind of global variable. So modifying it is not good way. So it would be better to update it temporarily like following:
from contextlib import contextmanager
@contextmanager
def sphinx_smartquotes_action(env):
try:
original = SphinxSmartQuotes.smartquotes_action
SphinxSmartQuotes.smart_quotes_action = env.config.smartquotes_action
yield
finally:
SphinxSmartQuotes.smart_quotes_action = original
# in env.doc_read() method:
with sphinx_smartquotes_action(self):
doctree = read_doc(...)
sphinx/environment/__init__.py
Outdated
@@ -692,6 +691,7 @@ def read_doc(self, docname, app=None): | |||
break | |||
else: | |||
self.settings['smart_quotes'] = False | |||
self.settings['smartquotes_action'] = self.config.smartquotes_action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for setting this into self.settings
?
docutils picks up transform modules by |
@tk0miya I have implemented your advices. I checked that SmartQuotes then applies also to Markdown files as added following the docs.
I removed it ;-) (I mean, I do not add it to |
sphinx/parsers.py
Outdated
@@ -55,15 +53,16 @@ def set_application(self, app): | |||
self.info = app.info | |||
|
|||
|
|||
class RSTParser(docutils.parsers.rst.Parser): | |||
class RSTParser(docutils.parsers.rst.Parser, Parser): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now inheriting from Parser is not needed, because we don't need access to app
. Should I delete that added inheritance ?
sphinx/io.py
Outdated
def __init__(self, app, parsers={}, *args, **kwargs): | ||
SphinxBaseReader.__init__(self, app, parsers, *args, **kwargs) | ||
if app.env.settings['smart_quotes']: | ||
self.transforms.append(SphinxSmartQuotes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this, also .md
source files (for example) are in the scope of Smart Quotes, which was not the case before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-)
sphinx/environment/__init__.py
Outdated
warnings.warn("'%s' (from '%s' in smartquotes_excludes) " | ||
"isn't a valid configuration variable. " | ||
"Did you forget an ending 's'?" | ||
% (valname[:-1], valname)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps the whole else
clause could be dropped if we limit to conditioning on language and builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I commented above, I can't imagine the use case for this else block.
I don't opposite it, but I'd like to know an example use case before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I overlooked your comment, I will remove that block as I don't see obvious use case either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
O="-E"`` to force re-parsing of source files, as the cached ones are | ||
already transformed. On the other hand the issue does not arise with | ||
direct usage of :program:`sphinx-build` as it caches | ||
(in its default usage) the parsed source files in per builder locations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we document like this our defects ;-) or leave it to user to discover ;-) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is internally and hard to understand behavior. So it should be documented. Let's go with as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits
sphinx/environment/__init__.py
Outdated
def sphinx_smartquotes_action(env): | ||
try: | ||
original = SphinxSmartQuotes.smartquotes_action | ||
setattr(SphinxSmartQuotes, 'smartquotes_action', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to using setattr
? An assignment is much simpler to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used an assignment but it did not have expected effect in my testing. Was it because SphinxSmartQuotes is imported separately in io.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... works perfectly well with an assignment. Don't know what went wrong (but I am doing various things at the same time, bad habit...). Ok, will push fix.
sphinx/io.py
Outdated
def __init__(self, app, parsers={}, *args, **kwargs): | ||
SphinxBaseReader.__init__(self, app, parsers, *args, **kwargs) | ||
if app.env.settings['smart_quotes']: | ||
self.transforms.append(SphinxSmartQuotes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please override get_transforms()
and add SphinxSmartQuotes
conditionally. because self.transforms
is a class variable. So SphinxSmartQuotes
will be added on each instantiation.
def __init__(...):
...
self.smart_quotes = app.env.settings['smart_quotes']
def get_transforms(self):
transforms = SphinxBaseReder.get_transforms()
if self.smart_quotes:
transforms.append(SphinxSmartQuotes)
return transforms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for my basic mistake. Indeed build time considerably increased due to it. I will fix.
sphinx/parsers.py
Outdated
@@ -55,15 +53,16 @@ def set_application(self, app): | |||
self.info = app.info | |||
|
|||
|
|||
class RSTParser(docutils.parsers.rst.Parser): | |||
class RSTParser(docutils.parsers.rst.Parser, Parser): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parser
super class is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
sphinx/environment/__init__.py
Outdated
@@ -96,10 +96,9 @@ | |||
def sphinx_smartquotes_action(env): | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the try is needed because Docutils SmartQuotes does not have the smartquotes_action
attribute at Docutils < 0.14.
sphinx/parsers.py
Outdated
@@ -53,7 +53,7 @@ def set_application(self, app): | |||
self.info = app.info | |||
|
|||
|
|||
class RSTParser(docutils.parsers.rst.Parser, Parser): | |||
class RSTParser(docutils.parsers.rst.Parser): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Parser inheritance was added in an earlier commit of this PR (#4360), but is now not needed
5ca4cee
to
85d1689
Compare
sphinx/environment/__init__.py
Outdated
original = SphinxSmartQuotes.smartquotes_action | ||
SphinxSmartQuotes.smartquotes_action = env.config.smartquotes_action | ||
yield | ||
SphinxSmartQuotes.smartquotes_action = original |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not executed when the error has happened at yield block. Usually, it does not effect to the processing because sphinx-build
crashes on the error. But the behavior might be changed in future. So it should be write back correctly using final
clause.
How about this implementation?
if not hasattr(SphinxSmartQuotes, 'smartquotes_action'):
# less than docutils-0.14
yield
else:
# docutils-0.14 or above
try:
original = SphinxSmartQuotes.smartquotes_action
SphinxSmartQuotes.smartquotes_action = env.config.smartquotes_action
finally:
SphinxSmartQuotes.smartquotes_action = original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I guess I add a yield
in the try
;-)
@@ -91,6 +92,17 @@ | |||
} # type: Dict[unicode, Union[bool, Callable]] | |||
|
|||
|
|||
@contextmanager | |||
def sphinx_smartquotes_action(env): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add type annotation: # type: (BuildEnvironment) -> Generator
.
closing and reopening to trigger again testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
CHANGES
Outdated
@@ -17,6 +17,10 @@ Features added | |||
* ``VerbatimHighlightColor`` is a new | |||
:ref:`LaTeX 'sphinxsetup' <latexsphinxsetup>` key (refs: #4285) | |||
* Easier customizability of LaTeX macros involved in rendering of code-blocks | |||
* Add :confval:`smart_quotes` to disable smart quotes through ``conf.py`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed smartquotes_disables
does not mentioned here.
Anyway, these settings for smart quotes are named smart_quotes
or smartquotes
. I feel they should have same prefix. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smartquotes_excludes
is mentioned at next line ...
I agree about using same prefix smartquotes
for all. Docutils has a smart_quotes
or smart-quotes
option and this why I originally used smart_quotes
. But Docutils has the smartquotes_action
class attribute hence I also followed. But I feel smartquotes
prefix for all conf.py variables is much better indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sorry > smartquotes_excludes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
CHANGES
Outdated
@@ -17,6 +17,10 @@ Features added | |||
* ``VerbatimHighlightColor`` is a new | |||
:ref:`LaTeX 'sphinxsetup' <latexsphinxsetup>` key (refs: #4285) | |||
* Easier customizability of LaTeX macros involved in rendering of code-blocks | |||
* Add :confval:`smart_quotes` to disable smart quotes through ``conf.py`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sorry > smartquotes_excludes
ping @jfbu Do you still have any remaining work for this PR? If none, could you merge this please? If you have tasks, I can wait it. So please let me know. Thanks, |
closes sphinx-doc#4142 closes sphinx-doc#4357 closes sphinx-doc#4359 refs: sphinx-doc#3967 Adds ``smartquotes``, ``smartquotes_action``, ``smartquotes_excludes`` configuration variables. - if ``smartquotes`` is set to False, then Smart Quotes transform is not applied even if a Docutils configuration file activates it, - the current default of ``smartquotes_excludes`` deactivates Smart Quotes for Japanese language, and also for the ``man`` and ``text`` builders. However, currently ``make text html`` deactivates Smart Quotes for ``html`` too, and ``make html text`` activates them for ``text`` too, because the picked environment is shared and already transformed. - now Smart Quotes get applied also when source documents are in Markdown or other formats.
48a6379
to
b90f2b0
Compare
b90f2b0
to
0d824df
Compare
I will merge once testing completes 🎉 |
Merging because Travis fails due to connect timeout errors. |
🎉 Great work! |
This closes #4359.
Relates #3967.