Skip to content

Commit

Permalink
Closes #1962: when adding directives, roles or nodes from an extensio…
Browse files Browse the repository at this point in the history
…n, warn if such

an element is already present (built-in or added by another extension).
  • Loading branch information
birkenfeld committed Jul 21, 2015
1 parent cb2f35e commit cdf0f82
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 2 deletions.
7 changes: 5 additions & 2 deletions CHANGES
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@ Incompatible changes
Features added
--------------

* #1962: when adding directives, roles or nodes from an extension, warn if such
an element is already present (built-in or added by another extension).

Bugs fixed
----------

* #1789: ``:pyobject:`` option of ``literalinclude`` directive includes following
lines after class definitions
* #1790: ``literalinclude`` strips empty lines at the head and tail
lines after class definitions.
* #1790: ``literalinclude`` strips empty lines at the head and tail.
* #1913: C++, fix assert bug for enumerators in next-to-global and global scope.

Documentation
Expand Down
20 changes: 20 additions & 0 deletions sphinx/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ def __init__(self, srcdir, confdir, outdir, doctreedir, buildername,
self._extensions = {}
self._extension_metadata = {}
self._listeners = {}
self._setting_up_extension = '?'
self.domains = BUILTIN_DOMAINS.copy()
self.buildername = buildername
self.builderclasses = BUILTIN_BUILDERS.copy()
Expand Down Expand Up @@ -144,6 +145,7 @@ def __init__(self, srcdir, confdir, outdir, doctreedir, buildername,
self.setup_extension(extension)
# the config file itself can be an extension
if self.config.setup:
self._setting_up_extension = 'conf.py'
# py31 doesn't have 'callable' function for below check
if hasattr(self.config.setup, '__call__'):
self.config.setup(self)
Expand Down Expand Up @@ -426,6 +428,7 @@ def setup_extension(self, extension):
self.debug('[app] setting up extension: %r', extension)
if extension in self._extensions:
return
self._setting_up_extension = extension
try:
mod = __import__(extension, None, None, ['setup'])
except ImportError as err:
Expand Down Expand Up @@ -460,6 +463,7 @@ def setup_extension(self, extension):
ext_meta = {'version': 'unknown version'}
self._extensions[extension] = mod
self._extension_metadata[extension] = ext_meta
self._setting_up_extension = '?'

def require_sphinx(self, version):
# check the Sphinx version if requested
Expand Down Expand Up @@ -550,6 +554,10 @@ def set_translator(self, name, translator_class):

def add_node(self, node, **kwds):
self.debug('[app] adding node: %r', (node, kwds))
if hasattr(nodes.GenericNodeVisitor, 'visit_' + node.__name__):
self.warn('while setting up extension %s: node class %r is '
'already registered, its visitors will be overridden' %
(self._setting_up_extension, node.__name__))
nodes._add_node_class_names([node.__name__])
for key, val in iteritems(kwds):
try:
Expand Down Expand Up @@ -594,17 +602,29 @@ def _directive_helper(self, obj, content=None, arguments=None, **options):
def add_directive(self, name, obj, content=None, arguments=None, **options):
self.debug('[app] adding directive: %r',
(name, obj, content, arguments, options))
if name in directives._directives:
self.warn('while setting up extension %s: directive %r is '
'already registered, it will be overridden' %
(self._setting_up_extension, name))
directives.register_directive(
name, self._directive_helper(obj, content, arguments, **options))

def add_role(self, name, role):
self.debug('[app] adding role: %r', (name, role))
if name in roles._roles:
self.warn('while setting up extension %s: role %r is '
'already registered, it will be overridden' %
(self._setting_up_extension, name))
roles.register_local_role(name, role)

def add_generic_role(self, name, nodeclass):
# don't use roles.register_generic_role because it uses
# register_canonical_role
self.debug('[app] adding generic role: %r', (name, nodeclass))
if name in roles._roles:
self.warn('while setting up extension %s: role %r is '
'already registered, it will be overridden' %
(self._setting_up_extension, name))
role = roles.GenericRole(name, nodeclass)
roles.register_local_role(name, role)

Expand Down

4 comments on commit cdf0f82

@shimizukawa
Copy link
Member

Choose a reason for hiding this comment

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

@birkenfeld
Copy link
Member Author

Choose a reason for hiding this comment

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

I saw it, but I did not have time to finish the fix :) sorry for the confusion.

@timgraham
Copy link
Contributor

@timgraham timgraham commented on cdf0f82 Mar 28, 2016

Choose a reason for hiding this comment

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

Could we add a type and subtype to the warn() calls so that these can be ignored using suppress_warnings? I'm thinking something like type='directive_overridden', subtype=name. For example, Django is overriding versionchanged and versionadded so we could have:

suppress_warnings = [
    'directive_overridden.versionadded',
    'directive_overridden.versionchanged',
]

@timgraham
Copy link
Contributor

Choose a reason for hiding this comment

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

Created a PR based on my previous comment: #2455, although the current solution introduces a test failure.

Please sign in to comment.