Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC3664: Pushrules for relations #3664
base: main
Are you sure you want to change the base?
MSC3664: Pushrules for relations #3664
Changes from 3 commits
d5f43bb
53a4607
fa8fc97
b90cdb6
a6eebe7
0161daa
242a65c
070babc
06b5393
f2a5c57
aebe3e9
1ead1da
6e1d1bb
3a288b6
df9aab5
00b0ef9
519b95d
c9d55e2
22e67c4
7248826
69fe03e
b1897fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This seems to implicitly usurp a relation type of
m.in_reply_to
, which might not be desired? I think there's been some thought these should be replaced eventually with MSC3267 (m.reference
relations).Regardless, it means that
m.in_reply_to
could not be used for a relation in the future and would need to be reserved.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.
Yes, the intention is to treat
m.in_reply_to
as a relation for now. #3440 actually makes it impossible to convert replies to normal relations easily, because it abuses the different format for fallback semantics. So we will probably have to live with this for a while (maybe #3051 can solve that), but I think this interface makes the most sense for now, while we dig ourselves deeper into this hole. We could also just name the replyrel_type
m.in_reply_to
, which is what I prefer (because reference is too generic) and then this rule would (hopefully) need no changes. :DThere 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.
If I've undersrood correctly, you could still use a relation type of
m.in_reply_to
, you'd just also get old-style replies mixed in to the results too? It would effectively reserve it for replies, but this probably ought to be the case anyway?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.
This is the bulk of my concern. (I think calling that out explicitly would probably be ideal?)
I don't think homeservers doesn't treat
m.in_reply_to
as a relation right now (at least Synapse doesn't), so this is quite a change in behavior in my opinion. (E.g. you can't query/relations
from MSC2675 using this)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.
Imo at least for the context of this MSC, they should be treated the same. It is a bit of a shame, that
/relations
doesn't tbh and maybe we should either allow that in the future or make replies proper relations, but I didn't want to affect any other behaviour with this MSC. I'll clarify.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.
Honestly this feels like putting a cart before a horse. It seems to assume that at some point we will replace replies with
"rel_type": "m.in_reply_to"
- which, afaik, hasn't even been proposed as an MSC, and as you note MSC3440 actually makes it harder to do so.It's kinda fine if we do make that change in the near term, but if we don't, then this is going to be a very strange thing for the spec to say.
This is all a horrible mess and I don't really know what to do about it :(.
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 still find it a bit hidden in this MSC that this grabs the
m.in_reply_to
as if it is a releation type when it isn't. I think this will be confusing for implementers (and difficult to specify properly).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.
Aren't these identical to:
(And a corresponding one for
m.thread
.)I'm not sure this example really shows the need for this MSC?
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.
Well, in the case of replies you need to also match, if it isn't a fallback. Also, it interacts better with #3051 or any other reorganization of the relation format. Matching dotted keys is also #3873. Yes, you can solve this differently, but that solution also has downsides. This is just a convenient extension of this MSC, the actual meat is matching the content of the related event.
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.
for reference:
.m.rule.invite_for_me
currently implements such a pattern.