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

Don't override the smart_quotes setting if it was already set #4112

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

alex
Copy link
Contributor

@alex alex commented Oct 4, 2017

This is needed to fix: sphinx-contrib/spelling#1

Rebase of #4110 onto the stable branch.

@tk0miya tk0miya merged commit 2ba4184 into sphinx-doc:stable Oct 4, 2017
@tk0miya
Copy link
Member

tk0miya commented Oct 4, 2017

Merged. Thank you for contribution!

@alex alex deleted the smart-quotes branch October 4, 2017 15:48
tk0miya added a commit that referenced this pull request Oct 4, 2017
tk0miya added a commit that referenced this pull request Oct 7, 2017
Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

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

for loop checking language tags should perhaps be executed to turn setting off, if it was on, but languages do not support smart quotes; or to avoid turn setting on if it was not set but language is "blacklisted" (CJK, #4142...)

if tag in smartchars.quotes:
break
else:
self.settings['smart_quotes'] = False
Copy link
Contributor

Choose a reason for hiding this comment

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

@tk0miya this may need some revision: if 'smart_quotes' was in self.settings and set to True then the for loop and its break and else clauses should arguably be executed, as they check if language supports smart quotes. Currently they are not executed if builder added to environment a 'smart_quotes' True setting

Also possibly this for loop could be occasion to check case that language is CJK language and then use False as default setting, not True. Relates #4142

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I missed that. Fixed at e03ec5b.

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

Successfully merging this pull request may close these issues.

3 participants