Fix #165: make the httpdomain renderer's whitespace delimited options usable - #176
Fix #165: make the httpdomain renderer's whitespace delimited options usable#176dstrodtman wants to merge 3 commits into
Conversation
Five of the httpdomain renderer's options map to 'None' in 'option_spec'. docutils treats 'None' as "option explicitly disabled" and raises KeyError for it, which the directive machinery reports as 'unknown option'. So 'response-examples-for', 'request-parameters-order' and the three example preference options could not be used at all. Give them the same whitespace splitting converter that 'http-methods-order' already had, which is what all five consumers of these values expect. The converter also tolerates an option passed with no value, where docutils hands over 'None' rather than a string. Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
The 'httpdomain' renderer's own directive name contains a colon, which Sphinx first reads as a domain prefix, so building a document that uses '.. openapi:httpdomain::' reports 'unknown directive name' before falling back to the registered name and rendering correctly anyway. Reaching the renderer through 'openapi_default_renderer' instead keeps the captured warning stream clean, so it says only what the tests are asserting on. Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
An empty value parsed to '[]', which for 'response-examples-for' is a present key that overrides the default and quietly disables every response example. Raise 'ValueError' instead, which docutils reports as a directive error. The options were previously accepted-looking but inert, so assert that each one changes rendered output rather than only that its converter parses. The test asserting the converter matched 'value.split()' was tautological, since that is the converter, and is now written against literal expectations. Also tee the captured warning stream to stderr, so that build warnings still reach pytest's capture for every other caller of the fixture. Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
|
Since this PR changes behavior for anyone already passing these options, I went looking for who that is. Short answer: as far as I can tell, nobody. I checked the twelve public projects I could find that depend on
None of them passes any of the six options this PR touches. None of them sets Method was per-repo code search for each option name. I ran a positive control first, searching each repo for Worth knowing which of them would pick up a release without doing anything: Two limits on this. It only covers projects discoverable through GitHub code search, so private repositories and anything hosted elsewhere aren't in it. And it says nothing about people who tried one of these options, found it did nothing, and removed it again, which is presumably how the original report came about. Happy to add a changelog note about the behavior change if you want one. |
Fixes #165.
Five options in
HttpdomainRenderer.option_specmap toNone:docutils reads
Nonein anoption_specas this option is explicitly disabled —assemble_option_dictraisesKeyErrorfor it exactly as it does for a name that is not in the spec at all, and the directive machinery turns that intounknown option: "response-examples-for". So none of the five could be used, and the renderer always fell back to its defaults.The fix is the one the reporter arrived at: give them a converter. All five consumers already expect a sequence of tokens —
response-examples-foris membership-tested against status codes,request-parameters-orderis lowercased and iterated, and the three preference options are passed to_iterinorderasorder_by— so the whitespace splitting thathttp-methods-orderalready used is what they want:Rather than repeat the lambda six times I pulled it out as
_split_optionand reused it forhttp-methods-ordertoo. It differs from the old lambda in one way: an empty value is rejected withValueError, which docutils reports as a proper directive error. Two reasons:None.split()raisedAttributeErroron an option written with no value, and since docutils only catchesValueError/TypeErroraround a converter, that escaped as a traceback rather than a directive error. That was a latent bug inhttp-methods-ordertoo.[]would be quietly destructive for exactly one option.self._response_examples_for = options.get("response-examples-for", self._response_examples_for)treats[]as a present value, so:response-examples-for:with nothing after it would override the default and disable every response example with no error at all. The other four are harmless when empty, but one uniform converter that rejects it seemed better than one that is safe five times out of six.A limitation worth naming
Whitespace splitting cannot express a media type that carries a parameter, so
:response-example-preference: application/json; charset=utf-8parses as['application/json;', 'charset=utf-8']and neither token ever matches. That affects only the three preference options, since the other three take HTTP methods, parameter locations and status codes. Writing the parameter without a space works today, and specs overwhelmingly keycontenton a bare media type, so I kept whitespace splitting for consistency withhttp-methods-orderrather than special-casing three of the six. If you would rather those three split on commas, say so and I will change them — it is a small diff, and better decided by you than guessed at by me.Measured on a real spec
Built a generated-looking spec against 0.9.0 installed from PyPI and against this branch, from a neutral working directory so the checkout couldn't shadow the released package:
Worth noting for anyone who tests this: without
-Wthe 0.9.0 build exits 0 while still printing that error and silently ignoring the option, which is how it can go unnoticed.Tests
tests/renderers/httpdomain/test_options.pyis new. Since the whole defect was that these options looked accepted while being inert, the tests that matter are the ones proving each option changes rendered output, so there is one per option: method order, parameter order,404examples appearing only onceresponse-examples-foris widened, and the preference options selectingtext/plainover theapplication/jsonthat wins by declaration order. One more asserts a request specific preference beats the shared one. Alongside those, each option is checked to parse to a literal expected list and to reject an empty value. 25 of the 28 cases fail before the change.The end-to-end half needed two small, additive things from the
run_sphinxfixture, with existing callers untouched:stderrrather than replacing it, so every other caller of the fixture still surfaces build warnings through pytest's capture on failure.unknown optiondoes failsphinx-build -Wwith exit 1, but it does not raise out ofapp.build()on Sphinx 8 or later — the warning is counted and the nonzero exit is applied by thesphinx.cmd.buildwrapper, so an API caller like this fixture sees a clean return. On Sphinx 7.4 it does raiseSphinxWarning, and the message lands in the exception rather than in the stream. Asserting on the captured warnings detects the regression on both.rendererparameter that writesopenapi_default_rendererinto the generatedconf.py, since these options only exist on thehttpdomainrenderer while the fixture builds.. openapi::.On the second point: I first had the fixture emit
.. openapi:httpdomain::instead, and that turned out to warnunknown directive name: openapi:httpdomainon Sphinx 9.0.4. Sphinx reads the colon as a domain prefix, fails to find anopenapidomain, warns, and then falls back to the registered name and renders correctly. Harmless for these tests, since the assertion is specific, but it puts a false warning in the stream the tests read, so going through the config value seemed better.Flagging it in case it is news, though it is unrelated to this fix. Sphinx reserves
:in a directive name for a domain prefix, soopenapi:httpdomainandopenapi:httpdomain:oldboth take that path, warn, and then fall through to the registered name. I tried Sphinx 7.4.7, 8.1.3, 8.2.3 and 9.0.4 — all four warn and all four render correctly, so this is long-standing rather than a recent change. Neither the colon names noropenapi_default_rendererappear in the README ordocs/, so I would guess few users reach it. Happy to file it if it is news to you, but it seemed too cosmetic to open an issue for unprompted.Full suite passes (521), and
pytest --regenerate-rendered-specs tests/produces no fixture diff, as expected since nothing about rendering changed.Happy to also document these five options in
docs/index.rstif you would like — I left them out because that page's option list is introduced as the options of theopenapidirective, and these are specific to thehttpdomainrenderer, so I did not want to guess at how you would rather structure it.http-methods-orderis documented there today with no such distinction.Context
Found while moving the Ray docs' Jobs API reference onto this extension. With the options unusable,
:response-examples-for:cannot be widened past the successful status codes; combined with #166 that reference currently ships with no example payloads at all.