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
MSC3266: Room summary API #3266
base: old_master
Are you sure you want to change the base?
MSC3266: Room summary API #3266
Changes from 12 commits
642f4e1
188b6e5
dc5b372
975ece5
d148acf
6776863
df376a3
43eecf0
66fee23
469b77b
04f807b
f1233c4
5fc2f5b
9e41b45
cab37e5
a93190f
8186b72
1a8ecff
82d8f3b
208a58c
33f3733
a5bc9ef
ac3d5da
dba6705
9719119
2ad832c
81fd904
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.
We appear to be missing error conditions from the MSC: what if the room doesn't exist? is invalid? unroutable?
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.
ok, but can we call it
room_summary
?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 don't really care about the name, but all the other endpoints are called /join/ and such as well, so I thought /summary/ would be more consistent with that, since it is followed by a room id/alias.
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 that for
/join
and/knock
, it's a bit more obvious that we're talking about a room./summary
could be anything, really.I'd be interested what other people think.
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 don't have a strong opinion on it, so we can change to to whatever, but I would appreciate some more opinions since otherwise I might need to flip flop between names again.
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.
let's call it
/room_summary
- there's a good chance we'll want other entities to be summarized in the future and it'd be annoying to fix/replace the API endpoint for them.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 true with any endpoint - the leak for this endpoint would more be that people could be surprised by the amount of metadata returned, such as in the case of matrix-org/matrix-spec#1186
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.
So should I just delete that line? I do think it is a genuine concern.
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 should also define a new
unstable_features
flag forGET /_matrix/client/versions
.For example, adapting from MSC3026:
Other MSCs that define an
unstable_features
are MSC2432, MSC2666, MSC2285, MSC3827, and MSC3440.