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

Ignore unknown directive options rather than aborting #199

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
- name: Install Python
uses: actions/setup-python@v2
with:
python-version: "3.x"
python-version: "3.6"
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: ‏remove this from the changeset, as the CI step for linting uses latest intentionally. Supported versions of Python are installed and tested against in the test job.
Also support for Python 3.6 was dropped from this library in #236

- name: Install dependencies
run: python -m pip install tox
- name: Run linting
Expand Down
32 changes: 32 additions & 0 deletions readme_renderer/rst.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,45 @@

import io

from docutils import utils
from docutils.core import publish_parts
from docutils.writers.html4css1 import HTMLTranslator, Writer
from docutils.utils import SystemMessage

from .clean import clean


def extract_extension_options(field_list, option_spec):
"""
Overrides `utils.extract_extension_options` and inlines
`utils.assemble_option_dict` to make it ignore unknown options passed to
directives (i.e. ``:caption:`` for ``.. code-block:``).
"""

dropped = set()
options = {}
for name, value in utils.extract_options(field_list):
convertor = option_spec.get(name)
if name in options or name in dropped:
raise utils.DuplicateOptionError('duplicate option "%s"' % name)

# silently drop unknown options as long as they are not duplicates
if convertor is None:
dropped.add(name)
continue
Comment on lines +40 to +43
Copy link
Member

Choose a reason for hiding this comment

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

question: ‏If I'm reading this correctly, this would drop any unknown options, not only the one surfaced as the code-block caption, correct?

Does this increase the possibility of errors being introduced for other known options? For example, if I mistyped the :header: option to :headers:, I get no warning/error due to this change, and the headers will be missing from the rendered output.

Would it be better to provide this function a specific list of known options we want to skip and thus keep the scope of the override narrow, at least until doctuils implements caption for code-block?


# continue as before
try:
options[name] = convertor(value)
except (ValueError, TypeError) as detail:
raise detail.__class__('(option: "%s"; value: %r)\n%s'
% (name, value, ' '.join(detail.args)))
return options


utils.extract_extension_options = extract_extension_options
Copy link
Member

@miketheman miketheman Jul 2, 2022

Choose a reason for hiding this comment

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

thought: ‏ I can't shake the feeling that this override is "wrong", despite it being completely valid and works. I spent some time trying to find other hooks in docutils to provide an option, a setting, or something to prevent needing to completely override a utility class that is used during rendering, but didn't find anything satisfying.

It works now, but if docutils ever changes the function, would we ever know?



class ReadMeHTMLTranslator(HTMLTranslator):

# Overrides base class not to output `<object>` tag for SVG images.
Expand Down
9 changes: 9 additions & 0 deletions tests/fixtures/test_rst_008.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,12 @@
<span class="k">fi</span>
</pre>
<p>or click <a href="http://www.surveymonkey.com" rel="nofollow">SurveyMonkey</a></p>
<p>and an ignored Sphinx option:</p>
<pre> <span class="k">def</span> <span class="nf">fib</span><span class="p">(</span><span class="n">n</span><span class="p">):</span>
<span class="k">if</span> <span class="n">n</span> <span class="o">&lt;</span> <span class="mi">1</span><span class="p">:</span>
<span class="k">return</span> <span class="mi">0</span>
<span class="k">elif</span> <span class="n">n</span> <span class="o">&lt;=</span> <span class="mi">2</span><span class="p">:</span>
<span class="k">return</span> <span class="mi">1</span>

<span class="k">return</span> <span class="n">fib</span><span class="p">(</span><span class="n">n</span> <span class="o">-</span> <span class="mi">1</span><span class="p">)</span> <span class="o">+</span> <span class="n">fib</span><span class="p">(</span><span class="n">n</span> <span class="o">-</span> <span class="mi">2</span><span class="p">)</span>
</pre>
13 changes: 13 additions & 0 deletions tests/fixtures/test_rst_008.rst
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,16 @@ and then here is some bash:
fi

or click `SurveyMonkey <http://www.surveymonkey.com>`_

and an ignored Sphinx option:

.. code-block:: python
:caption: Naive Fibonacci computation

def fib(n):
if n < 1:
return 0
elif n <= 2:
return 1

return fib(n - 1) + fib(n - 2)