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

Include information about additionalProperties in object tables #1798

Merged
merged 5 commits into from
May 2, 2024

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Apr 23, 2024

Currently, if we have an object which has additionalProperties in addition to properties, that information gets lost. This PR seeks to address that.

The individual commits should be separately reviewable.

Examples:

old:
image

new:
image

old:
image

new
image

old
image

new
image

Some of those descriptions could use improvement, but that is a separate job.

Preview: https://pr1798--matrix-spec-previews.netlify.app

@richvdh richvdh requested a review from a team as a code owner April 23, 2024 18:23
*
* Returns a dict containing:
*
* * `objects`: The array of object schema definitions.
*
* * `schema`: An updated copy of the `schema` input (eg, nested `allOf`
Copy link
Member Author

Choose a reason for hiding this comment

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

Since #1787, allOf entries are now resolved at the top level before we get here, rather than inside resolve-additional-types.

Just documenting the current situation.
…ct-table`

Previously, we were stripping `additionalProperties` and `patternProperties`
from all objects except top-level objects. Obviously, this was no good for
objects where a nested property contains such properties.

Fixing that (in `clean-object`) *ought* to be simple enough, except that it
turned out we were relying on the fact that would give us an "empty" entry in
the array of types-to-render-tables-for returned by
`resolve-additional-types`. (Normally, we don't want an object that only has
`additionalProperties` to have its own table, since we just embed it in the
parent table.)

So, we need to add more logic to `resolve-additional-types-inner` to suppress
such tables.

This commit doesn't change the rendered output at all (verified via `diff`):
it's just preparation for what comes next.
Copy link
Contributor

@zecakeh zecakeh left a comment

Choose a reason for hiding this comment

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

I almost worked on that today, until I realized we needed a way to show patternProperties too, which is why I worked on #1796 first 😉.

There is at least one TODO comment to remove at:

# TODO: TravisR - Make this render

layouts/partials/openapi/render-object-table.html Outdated Show resolved Hide resolved
layouts/partials/openapi/render-object-table.html Outdated Show resolved Hide resolved
@richvdh
Copy link
Member Author

richvdh commented Apr 24, 2024

I almost worked on that today, until I realized we needed a way to show patternProperties too, which is why I worked on #1796 first 😉.

Great minds think alike 😇. It's not the first time the two of us have ended up working on the same thing. I think we should probably try to mention what we're planning to work on in #matrix-docs:matrix.org in future.

@anoadragon453
Copy link
Member

Sorry for leaving this to sit for so long. These changes LGTM.

@richvdh richvdh merged commit 48f4c49 into main May 2, 2024
12 checks passed
@richvdh richvdh deleted the rav/additional_properties branch May 2, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants