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

Add support for pattern formats for patternProperties #1796

Merged
merged 6 commits into from Apr 24, 2024

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Apr 23, 2024

The idea here is to be able to identify more easily what strings represent in the rendered spec.

If this is accepted, then we can chose most additionalProperties to patternProperties throughout the specification. We can even extend the support to the regular format key on string properties.

Here is what that changes in the rendered specification, for m.receipt:

Before:

Capture d’écran du 2024-04-23 15-08-50

After:

image

Preview: https://pr1796--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 April 23, 2024 13:13
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

It looks really nice but I feel a bit uneasy that we end up with same regexes spelled out two times, in different files. And I might be missing something but you don't seem to actually use pattern defined in custom-formats.yaml?

@TheArcaneBrony
Copy link

Personally not sure if this change makes sense, given that column is meant to show the data type.
I wonder if something like {string user_id: Receipt data} would work better?

@zecakeh
Copy link
Contributor Author

zecakeh commented Apr 23, 2024

It looks really nice but I feel a bit uneasy that we end up with same regexes spelled out two times, in different files. And I might be missing something but you don't seem to actually use pattern defined in custom-formats.yaml?

As said in the docs of the file, the regex is only here as a reference to help for creating new patternProperties. It can safely be removed if it's bothering you.

data/custom-formats.yaml Outdated Show resolved Hide resolved
openapi_extensions.md Outdated Show resolved Hide resolved
layouts/partials/openapi/render-object-table.html Outdated Show resolved Hide resolved
layouts/partials/openapi/render-object-table.html Outdated Show resolved Hide resolved
data/custom-formats.yaml Outdated Show resolved Hide resolved
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
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.

LGTM!

@richvdh richvdh merged commit 2edfb21 into matrix-org:main Apr 24, 2024
12 checks passed
@zecakeh zecakeh deleted the pattern-formats branch April 24, 2024 12:20
@richvdh
Copy link
Member

richvdh commented Apr 30, 2024

Personally not sure if this change makes sense, given that column is meant to show the data type.
I wonder if something like {string user_id: Receipt data} would work better?

I realised we didn't reply to this (pro-tip: comments on the diff are more likely to get noticed than comments on the PR itself). Given this is JSON, the only thing that can be used as an object key is a string. I don't think there is any need to specify string for every such mapping.

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

4 participants