Give a warning when extensions are explicitly not parallel safe #6812
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.0 #6812 +/- ##
==========================================
+ Coverage 83.73% 83.73% +<.01%
==========================================
Files 271 271
Lines 41169 41175 +6
Branches 6011 6011
==========================================
+ Hits 34471 34477 +6
Misses 5367 5367
Partials 1331 1331
Continue to review full report at Codecov.
|
sphinx/application.py
Outdated
| @@ -1126,28 +1126,33 @@ def is_parallel_allowed(self, typ): | |||
|
|
|||
| ``typ`` is a type of processing; ``'read'`` or ``'write'``. | |||
| """ | |||
|
|
|||
There was a problem hiding this comment.
Could you remove this empty line please?
sphinx/application.py
Outdated
| "for parallel reading, assuming it isn't - please " | ||
| "ask the extension author to check and make it " | ||
| "explicit") | ||
| message_false = "the %s extension is not safe for parallel reading" |
There was a problem hiding this comment.
Please wrap with _() function like above. It is needed for i18n.
sphinx/application.py
Outdated
| else: | ||
| raise ValueError('parallel type %s is not supported' % typ) | ||
|
|
||
| for ext in self.extensions.values(): | ||
| allowed = getattr(ext, attrname, None) | ||
| if allowed is None: | ||
| logger.warning(message, ext.name) | ||
| logger.warning(message_none, ext.name) |
There was a problem hiding this comment.
I feel message_none and message_false not describe its purpose. I think message_not_declared and message_not_allowed are better. What do you think?
There was a problem hiding this comment.
Fixed - I settled on message_not_declared and message_not_safe - is that ok?
tests/test_application.py
Outdated
| @@ -98,6 +98,8 @@ def test_add_is_parallel_allowed(app, status, warning): | |||
|
|
|||
| app.setup_extension('read_serial') | |||
| assert app.is_parallel_allowed('read') is False | |||
| assert ("the read_serial extension is not safe for parallel reading") in warning.getvalue() | |||
There was a problem hiding this comment.
Coudl you remove parenthesis please? They are used only for folding long text to multiple lines.
astrofrog
force-pushed
the
explicit-parallel-unsafe
branch
from
9922698
to
0e51478
Compare
astrofrog
force-pushed
the
explicit-parallel-unsafe
branch
from
0e51478
to
fa1cca1
Compare
|
@tk0miya - thanks for the review! I've addressed all your comments and changed this PR to be against 2.0. |
|
Merged. |
|
Thanks! |
Feature or Bugfix
I would personally treat this as a bug fix since the previous behavior was confusing.
Purpose
Currently, if building a set of docs with a number of extensions in parallel, if one of the extensions explicitly declares itself to be unsafe, the build falls back to a serial build silently, with no way of knowing what extension caused this. Rather than have to dig into the source code for each extension, it's much simpler to just warn the user if the build is falling back to a serial build and what extension caused this. I think it makes sense to include this since if the user is opting in to a parallel build, anything that prevents that from happening should be explicitly stated.