Add event_property_is and event_property_contains props to PushConditions #1673
Conversation
…ions Signed-off-by: tusooa <tusooa@kazv.moe>
There was a problem hiding this comment.
Thanks for the contribution!
Does this relate to a particular MSC? If so, please could you link it?
Also, please be aware that, as per CONTRIBUTING.rst, we require sign-off from a legally recognised name. If you'd prefer to keep this contribution anonymous, please follow the instructions there for private sign-off.
|
For cross-linking: seems I missed this in #1464? |
The creator of DCO clarified that requiring a "legal name" is not an intention. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330 I recognize that Matrix may have different intentions on this matter, but I will NOT provide a "legal name" to anyone just for the sake of ME contributing code to others, even if it is done privately. Requiring me to provide "legal name" is a great way to ensure that I will NEVER contribute code to any matrix-org repositories again. If desired, please close this one and all other PRs I have made against any matrix-org repo, and roll back #1671 . |
|
@tusooa Thank you so much for your contribution and your feedback on the DCO! The team here will follow up with you about this PR in the next couple of days, but I wanted to jump in as the Managing Director of the Foundation to speak to the policy. Coincidentally, I was just discussing the specifics of our policy with the team last week! We are broadly on the same page about the issues with real name/legal name policies and are looking at amending our policy accordingly :) Not clear on timeline yet – got a lot of plates we're spinning – but I thought you deserved to know. |
|
Hi @tusooa, thanks for your patience on this. I've checked in with our legal team and although our policy is currently still that we prefer to have a legal name, we're prepared to grant exceptions in cases where sharing a real name is a particular concern. And as @joshsimmons already said, he and the legal team are working on making the DCO process more inclusive generally. On that basis, your existing sign-off is sufficient, thank you! |
Right. So these properties were added in matrix-org/matrix-spec-proposals#3758 and matrix-org/matrix-spec-proposals#3966. |
| @@ -43,5 +43,11 @@ properties: | |||
| optionally prefixed by one of, ==, <, >, >= or <=. A prefix of < matches | |||
| rooms where the member count is strictly less than the given number and | |||
| so forth. If no prefix is present, this parameter defaults to ==. | |||
| value: | |||
| type: ["string", "integer", "boolean", "null"] | |||
There was a problem hiding this comment.
Hrm. The spec displays this in a way that implies it should be an array. It looks like https://github.com/matrix-org/matrix-spec/blob/main/layouts/partials/openapi/render-object-table.html needs an update to handle this case.
There was a problem hiding this comment.
Wait. According to https://swagger.io/docs/specification/data-models/data-types/, this is not valid. "type as a list is not valid in OpenAPI (even though it is valid in JSON Schema)".
However, it is definitely a pattern we use elsewhere, such as here.
@KitsuneRal or @zecakeh, as people who know a lot more about OpenAPI than me: can you comment on this? Should we be updating the render-object-table to support multi-valued types? Or should we be using oneOf here?
There was a problem hiding this comment.
It was not valid in OpenAPI 3.0, it is valid in OpenAPI 3.1, which is fully compatible with JSON Schema.
From "Migrating from OpenAPI 3.0 to 3.1.0":
In line with JSON Schema, the
typekeyword can now define multiple types for a schema with an array.
There was a problem hiding this comment.
Aha, thanks for the clarification; that's very helpful.
In that case I guess we should merge this and file a separate issue to fix the template.
| Required for `event_property_is` and `event_property_contains` conditions. | ||
| A non-compound [canonical JSON](/appendices#canonical-json) value to match | ||
| against. |
There was a problem hiding this comment.
For the record:
I don't find the wording "a non-compound canonical JSON value" particularly clear, but it does match the existing wording in the Conditions section, so let's go with it.
|
Thanks again for the contribution @tusooa ! |
Preview: https://pr1673--matrix-spec-previews.netlify.app