-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
Remove the dont_notify
and coalesce
push rule actions.
#1501
Conversation
Per MSC3987, these should both be considered no-ops.
{{% boxes/note %}} | ||
Older versions of the Matrix specification included the `dont_notify` and | ||
`coalesce` actions. These should both be considered no-ops (ignored, not | ||
rejected) if received from a client. | ||
{{% /boxes/note %}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fear is that someone will reject a push rule with actions of ["dont_notify"]
, which is equivalent to []
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to evaluate this change for a system where 1.3 and 1.11 servers will co-exists and am wondering: In terms of RFC2119-language, would the "ignoring without rejecting" behavior be a MUST or a SHOULD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a MUST, old clients might still use dont_notify
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that would make sense. Contrary to what I wrote in #1782 (comment) this morning this might actually be a case where keyword usage in an info box would be helpful.
(This is a draft since the MSC hasn't actually passed FCP yet.) |
FCP has finished, putting up for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change that you've made looks good, but I think we should also be removing the places where the rules are used, e.g. in https://spec.matrix.org/v1.6/client-server-api/#predefined-rules
A grep says that there's 5 more references to dont_notify
in content/client-server-api/modules/push.md
, and it's also in data/api/client-server/pushrules.yaml
(3x) and in data/event-schemas/examples/m.push_rules.yaml
(2x)
Oops, I forgot to do that. Thanks for noticing! 👍 |
Per MSC3987, these should both be considered no-ops.
Preview: https://pr1501--matrix-spec-previews.netlify.app