Closed kevindew closed 3 years ago
@ChrisBAshton Thanks for the review and apologies in taking so long to get back to this. I've fixed up various bits noted.
@ChrisBAshton about 1 million years later, but this is good for a re-review if you don't mind? 🙏
Following up from https://github.com/alphagov/tech-docs-gem/pull/121 this updates openapi_parser to allow the latest version of gem. Since the gem is below version 1 it seemed safe to maintain the pin.
I've also done some cleaning up of some of the Ruby code in
GovukTechDocs::ApiReference::Renderer
to make it more shorter and more consistent with modern Ruby. There's a few dubious things in there that I left as I didn't want to risk creating fresh bugs from "fixing" stuff.I've also rebased https://github.com/alphagov/tech-docs-gem/pull/113 onto this as that fix would conflict with the changes here.
Edit:
Since opening this @tijmenb closed #113 so the link to that is no longer accurate. I didn't want to re-instate a bug by undoing that so wanted to keep that change here and had already amended his change to use a new method in openapi_parser to check required status.
To document the issue, an OpenAPI document schema object has a required property which must be an array of strings which is used to indicate what properties of that object are required.
Previously the schema template just checked whether that array was set to determine if a parameter was required: https://github.com/alphagov/tech-docs-gem/blob/a2ba6143342ea9b58fef879a3946500fba71db23/lib/govuk_tech_docs/api_reference/templates/schema.html.erb#L13 This is incorrect as this only tells you whether that parameter requires properties of its own.
Instead we use the
#requires?
method on the schema to determine if this property is required.