-
Notifications
You must be signed in to change notification settings - Fork 90
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Does this increase the possibility of errors being introduced for other known options? For example, if I mistyped the 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 |
||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
There was a problem hiding this comment.
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