Skip to content

Add support for pattern formats for patternProperties#1796

Merged
richvdh merged 6 commits intomatrix-org:mainfrom
zecakeh:pattern-formats
Apr 24, 2024
Merged

Add support for pattern formats for patternProperties#1796
richvdh merged 6 commits intomatrix-org:mainfrom
zecakeh:pattern-formats

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.

zecakeh added 4 commits April 23, 2024 19:24
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
@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.

4 participants