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
MSC2666: Get rooms in common with another user #2666
base: old_master
Are you sure you want to change the base?
MSC2666: Get rooms in common with another user #2666
Changes from all commits
c61790e
4264f32
008951f
29f02ed
2b75da8
630af1c
d885bcf
5254076
3f2faef
db99583
10a2df2
d3b17e6
4ac7ce8
a4f5bae
c453704
cd173d5
1a389f9
fbbb2d9
591d3e5
a1de65f
d59d051
6a4e523
b946cc3
ea49670
6f4f01b
60ae94f
7829c3b
92aef5b
d58d0a1
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.
If
user_id
can't be specified multiple times, it might belong in the HTTP path instead. Also, can it be specified multiple times?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.
ah, it's listed later in the MSC. Is there a technical reason preventing multiple users from being searched? It feels like an awkward thing to make a future MSC for when we're already here.
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 discussion context is here: #2666 (comment)
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.
There is no technical reason preventing multiple users for being searched, but I don't see a convincing client usecase, while I agreed to keep the path open for one if the time came, to make it easily extendible.
In the past, this MSC had path-element handling, but I got convinced that that was a leftover pattern of an older way of handling things, and doing things via query was a newer way, so I applied that.
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 think if we're not putting the user id in the path, then we shouldn't use the
/_matrix/client/v1/user
hierarchy, since most of the endpoints under that hierarchy do have a user_id as the next path component.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 also realising that if we want to be able to get rooms in common with more users, there could just be another version of this endpoint.
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.
With the current MSC text (see "Forward-compatibility considerations"), this seems to be resolved?
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 want some clear direction on this. The current version (that I merge in this branch via #4035) changes some labels around enough that i'd have to give it another unstable_features tag.
I haven't yet, as i want to sweep the result of this in that tag, before i propose it FCP again.
Should I change this back to a path component, or should I keep this as a query element endpoint? I can do either.
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.
It may also be worth reading #4024 to match the language there, as this seems like the sort of feature a client would want to use between FCP completing and spec release.
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.
Ah, I think with this original language I remember/wrote this down to "consider stable" to be "when it is included in a spec version"
Together with richvdh's comment here, i think that for this MSC it is unnecessary to introduce a
.stable
unstable_features
flag.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.
/res