From 5cc72ca52bf42239a4bf1eb8df276545e886198e Mon Sep 17 00:00:00 2001 From: Robert Lehmann Date: Thu, 30 Oct 2014 15:46:02 +0100 Subject: [PATCH 1/7] Checked configuration values for their types. Fixes #1150. --- sphinx/application.py | 3 +++ sphinx/config.py | 27 +++++++++++++++++++++++++-- sphinx/errors.py | 10 ++++++++-- tests/test_config.py | 8 ++++++++ 4 files changed, 44 insertions(+), 4 deletions(-) diff --git a/sphinx/application.py b/sphinx/application.py index 630bff36f..5f6b92f50 100644 --- a/sphinx/application.py +++ b/sphinx/application.py @@ -124,6 +124,7 @@ class Sphinx(object): self.config = Config(confdir, CONFIG_FILENAME, confoverrides or {}, self.tags) self.config.check_unicode(self.warn) + # defer checking types until i18n has been initialized # set confdir to srcdir if -C given (!= no confdir); a few pieces # of code expect a confdir to be set @@ -172,6 +173,8 @@ class Sphinx(object): # set up translation infrastructure self._init_i18n() + # check all configuration values for permissible types + self.config.check_types(self.warn) # set up the build environment self._init_env(freshenv) # set up the builder diff --git a/sphinx/config.py b/sphinx/config.py index cf6bed087..be30d1ea4 100644 --- a/sphinx/config.py +++ b/sphinx/config.py @@ -14,7 +14,7 @@ from os import path from six import PY3, iteritems, string_types, binary_type, integer_types -from sphinx.errors import ConfigError +from sphinx.errors import ConfigError, ConfigWarning from sphinx.locale import l_ from sphinx.util.osutil import make_filename, cd from sphinx.util.pycompat import execfile_ @@ -249,6 +249,30 @@ class Config(object): self.setup = config.get('setup', None) self.extensions = config.get('extensions', []) + def check_types(self, warn): + # check all values for deviation from the default value's type, since + # that can result in TypeErrors all over the place + # NB. since config values might use l_() we have to wait with calling + # this method until i18n is initialized + for name in self._raw_config: + if name not in Config.config_values: + continue # we don't know a default value + default, dummy_rebuild = Config.config_values[name] + if hasattr(default, '__call__'): + default = default(self) # could invoke l_() + if default is None: + continue + current = self[name] + if type(current) is type(default): + continue + common_bases = ( + set(type(current).__bases__) & set(type(default).__bases__)) + common_bases.discard(object) + if common_bases: + continue # at least we share a non-trivial base class + warn("the config value %r has type `%s', defaults to `%s.'" + % (name, type(current).__name__, type(default).__name__)) + def check_unicode(self, warn): # check all string values for non-ASCII characters in bytestrings, # since that can result in UnicodeErrors all over the place @@ -296,7 +320,6 @@ class Config(object): for name in config: if name in self.values: self.__dict__[name] = config[name] - del self._raw_config def __getattr__(self, name): if name.startswith('_'): diff --git a/sphinx/errors.py b/sphinx/errors.py index 3d7a5eb47..4e9828dc2 100644 --- a/sphinx/errors.py +++ b/sphinx/errors.py @@ -3,8 +3,8 @@ sphinx.errors ~~~~~~~~~~~~~ - Contains SphinxError and a few subclasses (in an extra module to avoid - circular import problems). + Contains SphinxError, a few subclasses (in an extra module to avoid + circular import problems), and related classes. :copyright: Copyright 2007-2014 by the Sphinx team, see AUTHORS. :license: BSD, see LICENSE for details. @@ -75,3 +75,9 @@ class SphinxParallelError(Exception): def __str__(self): return traceback.format_exception_only( self.orig_exc.__class__, self.orig_exc)[0].strip() + +class ConfigWarning(UserWarning): + """ + Base category for warnings about dubious configuration values. + """ + pass diff --git a/tests/test_config.py b/tests/test_config.py index 0dcf3fa3e..c11c07219 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -133,3 +133,11 @@ def test_config_eol(tmpdir): cfg = Config(tmpdir, 'conf.py', {}, None) cfg.init_values(lambda warning: 1/0) assert cfg.project == u'spam' + + +@with_app(confoverrides={'master_doc': 123}) +def test_check_types(app, status, warning): + # WARNING: the config value 'master_doc' has type `int', defaults to `str.' + assert any(buf.startswith('WARNING:') + and 'master_doc' in buf and 'int' in buf and 'str' in buf + for buf in warning.buflist) From 872f35f1139670025164e13456e056c9c1f1c7c6 Mon Sep 17 00:00:00 2001 From: Robert Lehmann Date: Thu, 30 Oct 2014 15:59:14 +0100 Subject: [PATCH 2/7] Remove superfluous warning category. --- sphinx/errors.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/sphinx/errors.py b/sphinx/errors.py index 4e9828dc2..3d7a5eb47 100644 --- a/sphinx/errors.py +++ b/sphinx/errors.py @@ -3,8 +3,8 @@ sphinx.errors ~~~~~~~~~~~~~ - Contains SphinxError, a few subclasses (in an extra module to avoid - circular import problems), and related classes. + Contains SphinxError and a few subclasses (in an extra module to avoid + circular import problems). :copyright: Copyright 2007-2014 by the Sphinx team, see AUTHORS. :license: BSD, see LICENSE for details. @@ -75,9 +75,3 @@ class SphinxParallelError(Exception): def __str__(self): return traceback.format_exception_only( self.orig_exc.__class__, self.orig_exc)[0].strip() - -class ConfigWarning(UserWarning): - """ - Base category for warnings about dubious configuration values. - """ - pass From f607414034424ef19d2544f0879f4555b64346cd Mon Sep 17 00:00:00 2001 From: Robert Lehmann Date: Fri, 31 Oct 2014 11:13:45 +0100 Subject: [PATCH 3/7] Remove references to outdated warning category. --- sphinx/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sphinx/config.py b/sphinx/config.py index be30d1ea4..26bd76971 100644 --- a/sphinx/config.py +++ b/sphinx/config.py @@ -14,7 +14,7 @@ from os import path from six import PY3, iteritems, string_types, binary_type, integer_types -from sphinx.errors import ConfigError, ConfigWarning +from sphinx.errors import ConfigError from sphinx.locale import l_ from sphinx.util.osutil import make_filename, cd from sphinx.util.pycompat import execfile_ From e1a211c414fd4f099d3ebe1c21fb39ce3715807a Mon Sep 17 00:00:00 2001 From: Robert Lehmann Date: Fri, 31 Oct 2014 11:14:14 +0100 Subject: [PATCH 4/7] Changelog for PR#314. --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 6a2da9e66..5d72d8b0d 100644 --- a/CHANGES +++ b/CHANGES @@ -9,6 +9,9 @@ Features added * #1597: Added possibility to return a new template name from `html-page-context`. +* PR#314, #1150: Configuration values are now checked for their type. A + warning is raised if the configured and the default value do not have the + same type and do not share a common non-trivial base class. Bugs fixed ---------- From 49d9efa13b065328d40a5cecfc178307236f2a43 Mon Sep 17 00:00:00 2001 From: Robert Lehmann Date: Wed, 5 Nov 2014 10:52:04 +0100 Subject: [PATCH 5/7] Fix type checking for subclass scenarios. --- sphinx/config.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sphinx/config.py b/sphinx/config.py index 26bd76971..b8a3d6648 100644 --- a/sphinx/config.py +++ b/sphinx/config.py @@ -224,7 +224,7 @@ class Config(object): self.overrides = overrides self.values = Config.config_values.copy() config = {} - if 'extensions' in overrides: + if 'extensions' in overrides: #XXX do we need this? if isinstance(overrides['extensions'], string_types): config['extensions'] = overrides.pop('extensions').split(',') else: @@ -265,8 +265,8 @@ class Config(object): current = self[name] if type(current) is type(default): continue - common_bases = ( - set(type(current).__bases__) & set(type(default).__bases__)) + common_bases = (set(type(current).__bases__ + (type(current),)) + & set(type(default).__bases__)) common_bases.discard(object) if common_bases: continue # at least we share a non-trivial base class From ed5f77916f70de418b3854c6f61be461cc4d1459 Mon Sep 17 00:00:00 2001 From: Robert Lehmann Date: Wed, 5 Nov 2014 14:33:30 +0100 Subject: [PATCH 6/7] Spec out configuration type checks. --- tests/test_config.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/test_config.py b/tests/test_config.py index c11c07219..63a083f97 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -141,3 +141,32 @@ def test_check_types(app, status, warning): assert any(buf.startswith('WARNING:') and 'master_doc' in buf and 'int' in buf and 'str' in buf for buf in warning.buflist) + +@with_app(confoverrides={'man_pages': 123}) +def test_check_types_lambda(app, status, warning): + # WARNING: the config value 'man_pages' has type `int', defaults to `list.' + assert any(buf.startswith('WARNING:') + and 'man_pages' in buf and 'int' in buf and 'list' in buf + for buf in warning.buflist) + +@with_app(confoverrides={'man_pages': []}) +def test_check_types_lambda_negative(app, status, warning): + assert not any(buf.startswith('WARNING:') and 'man_pages' in buf + for buf in warning.buflist) + +@with_app(confoverrides={'epub_tocdepth': True}) +def test_check_types_child(app, status, warning): + # WARNING: the config value 'master_doc' has type `bool', defaults to `int.' + assert any(buf.startswith('WARNING:') + and 'epub_tocdepth' in buf and 'bool' in buf and 'int' in buf + for buf in warning.buflist) + +@with_app(confoverrides={'nitpicky': 3}) +def test_check_types_parent(app, status, warning): + assert not any(buf.startswith('WARNING:') and 'nitpicky' in buf + for buf in warning.buflist) + +@with_app(confoverrides={'html_add_permalinks': 'bar'}) +def test_check_types_sibling(app, status, warning): + assert not any(buf.startswith('WARNING:') and 'html_add_permalinks' in buf + for buf in warning.buflist) From c528b65292fd19642f1ce4e36e77cc6a3b4d1ab5 Mon Sep 17 00:00:00 2001 From: Robert Lehmann Date: Sun, 9 Nov 2014 09:55:56 +0100 Subject: [PATCH 7/7] Refactor tests. --- tests/test_config.py | 67 ++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/tests/test_config.py b/tests/test_config.py index 63a083f97..e48079e3a 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -9,7 +9,7 @@ :copyright: Copyright 2007-2014 by the Sphinx team, see AUTHORS. :license: BSD, see LICENSE for details. """ -from six import PY3 +from six import PY2, PY3, StringIO from util import TestApp, with_app, with_tempdir, raises, raises_msg @@ -135,38 +135,39 @@ def test_config_eol(tmpdir): assert cfg.project == u'spam' -@with_app(confoverrides={'master_doc': 123}) -def test_check_types(app, status, warning): - # WARNING: the config value 'master_doc' has type `int', defaults to `str.' - assert any(buf.startswith('WARNING:') - and 'master_doc' in buf and 'int' in buf and 'str' in buf - for buf in warning.buflist) +TYPECHECK_OVERRIDES = [ + # configuration key, override value, should warn, default type + ('master_doc', 123, True, str), + ('man_pages', 123, True, list), # lambda + ('man_pages', [], False, list), + ('epub_tocdepth', True, True, int), # child type + ('nitpicky', 3, False, bool), # parent type + ('templates_path', (), True, list), # other sequence, also raises +] +if PY2: + # Run a check for proper sibling detection in Python 2. Under py3k, the + # default types do not have any siblings. + TYPECHECK_OVERRIDES.append( + ('html_add_permalinks', 'bar', False, unicode)) -@with_app(confoverrides={'man_pages': 123}) -def test_check_types_lambda(app, status, warning): - # WARNING: the config value 'man_pages' has type `int', defaults to `list.' - assert any(buf.startswith('WARNING:') - and 'man_pages' in buf and 'int' in buf and 'list' in buf - for buf in warning.buflist) +def test_gen_check_types(): + for key, value, should, deftype in TYPECHECK_OVERRIDES: + warning = StringIO() + try: + app = TestApp(confoverrides={key: value}, warning=warning) + except: + pass + else: + app.cleanup() -@with_app(confoverrides={'man_pages': []}) -def test_check_types_lambda_negative(app, status, warning): - assert not any(buf.startswith('WARNING:') and 'man_pages' in buf - for buf in warning.buflist) + real = type(value).__name__ + msg = ("WARNING: the config value %r has type `%s'," + " defaults to `%s.'\n" % (key, real, deftype.__name__)) + def test(): + assert (msg in warning.buflist) == should, \ + "Setting %s to %r should%s raise: %s" % \ + (key, value, " not" if should else "", msg) + test.description = "test_check_type_%s_on_%s" % \ + (real, type(Config.config_values[key][0]).__name__) -@with_app(confoverrides={'epub_tocdepth': True}) -def test_check_types_child(app, status, warning): - # WARNING: the config value 'master_doc' has type `bool', defaults to `int.' - assert any(buf.startswith('WARNING:') - and 'epub_tocdepth' in buf and 'bool' in buf and 'int' in buf - for buf in warning.buflist) - -@with_app(confoverrides={'nitpicky': 3}) -def test_check_types_parent(app, status, warning): - assert not any(buf.startswith('WARNING:') and 'nitpicky' in buf - for buf in warning.buflist) - -@with_app(confoverrides={'html_add_permalinks': 'bar'}) -def test_check_types_sibling(app, status, warning): - assert not any(buf.startswith('WARNING:') and 'html_add_permalinks' in buf - for buf in warning.buflist) + yield test