Ignore unknown directive options rather than aborting#199
Ignore unknown directive options rather than aborting#199terencehonles wants to merge 2 commits intopypa:mainfrom
Conversation
|
I didn't look at the code too closely, but it may make sense to see if these can surfaced as warnings which would still fail with |
|
@di not sure if you have any input on this, sorry to bother if you're the wrong person to ask. |
There was a problem hiding this comment.
Reviewing older PRs, and took some time to discover some history and docutils - good learning on my part!
A few notes inline, using Conventional Comments style.
In general, it needs a rebase against current main since we added type hints there's a conflict.
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: "3.x" | ||
| python-version: "3.6" |
There was a problem hiding this comment.
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
| return options | ||
|
|
||
|
|
||
| utils.extract_extension_options = extract_extension_options |
There was a problem hiding this comment.
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?
| # silently drop unknown options as long as they are not duplicates | ||
| if convertor is None: | ||
| dropped.add(name) | ||
| continue |
There was a problem hiding this comment.
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?
|
Related to #160 |
2 participants
This is the same as my PR github/markup#1452 and it may make sense to incorporate other changes from GitHub's rST processing.