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

Do not add empty arrays to examples #1849

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Jun 7, 2024

Effectively, this only changes the rendering of the example of https://spec.matrix.org/v1.10/application-service-api/#registration

Result

Pull Request Checklist

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

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@zecakeh zecakeh requested a review from a team as a code owner June 7, 2024 12:58
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Copy link
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

image

this doesn't look like it was the intended result...

And are we sure that there aren't any places where an empty list is significant in the spec? I feel like I've seen some places where it says something like "an empty list represents explicitly nothing, whereas null/omitted represents use the defaults".

@zecakeh
Copy link
Contributor Author

zecakeh commented Jun 8, 2024

It was the intended result for me. Especially getting rid of [ null ] which is always wrong. If we want to have a list it is always possible to provide it as an example in the schema, but without this change it is not possible to remove empty lists that have no examples.

The fact that this example is still wrong is another issue. A complete example should probably be provided since we cannot generate a valid one with the current schema.

IMO, this brings to light the issue that we don't check those generated examples in CI. To do that we would need to re-implement the example generation for CI. Or to stop relying on generated examples and always provide full examples, because they are easier to check. The thing though is that we rely heavily on them.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Seems sensible. We should have a better example for "Registration" but that's a separate problem.

@richvdh richvdh merged commit acec09f into matrix-org:main Jun 11, 2024
12 checks passed
@zecakeh zecakeh deleted the no-empty-example-array branch June 11, 2024 16:20
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