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

New /publicRoom APIs #388

Merged
merged 20 commits into from Nov 21, 2016

Conversation

Projects
None yet
3 participants
@erikjohnston
Member

erikjohnston commented Sep 29, 2016

No description provided.

@erikjohnston erikjohnston changed the title from Erikj/public rooms to New /publicRoom APIs Sep 29, 2016

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Sep 29, 2016

@dbkr Can you just sanity check that it matches your understanding?

@richvdh

Some nit-pickery. I realise that partly I'm requesting changes to existing stuff, but (a) it's a good opportunity to fix it, and (b) you're c&ping the broken stuff and I'd prefer not to see it spread.

A bunch of the comments apply to the POST version as well as the GET version.

canonical_alias:
type: string
description: |-
The canonical alias of the room, if any. May be null.

This comment has been minimized.

@richvdh

richvdh Sep 29, 2016

Member

Null, but not absent? I thought we avoided nulls in the API. If it really can be null rather than absent, it needs adding to required and to the example.

Ditto name, topic, etc, and ditto in the POST API.

This comment has been minimized.

@erikjohnston

erikjohnston Sep 30, 2016

Member

Changed to be optional rather than null.

type: string
description: |-
The name of the room, if any. May be null.
num_joined_members:

This comment has been minimized.

@richvdh

richvdh Sep 29, 2016

Member

is this required? Needs adding to required if so; otherwise could do with explictly saying it's optional.

This comment has been minimized.

@erikjohnston
type: string
description: |-
A pagination token for the response, if there are any more results.
total_room_count_estimate:

This comment has been minimized.

@richvdh

richvdh Sep 29, 2016

Member

required? optional?

400:
description: >
The request body is malformed or the room alias specified is already taken.
post:

This comment has been minimized.

@richvdh

richvdh Sep 29, 2016

Member

feels like POST /_matrix/client/unstable/publicRooms should be for adding rooms to the list. I don't really have any better suggestions though :/.

This comment has been minimized.

@erikjohnston

erikjohnston Sep 29, 2016

Member

Agreed :/

type: string
description: |-
A pagination token for the response, if there are any more results.
prev_batch:

This comment has been minimized.

@richvdh

richvdh Sep 29, 2016

Member

what's this for? next_batch I get, but not prev_batch. It sounds like there might be requirements on the server which aren't being made explicit here?

description: |-
Optional filtering of the returned rooms.
properties:
generic_search_term:

This comment has been minimized.

@richvdh

richvdh Sep 29, 2016

Member

required? optional?

This comment has been minimized.

@erikjohnston

erikjohnston Sep 29, 2016

Member

I appended (Optional) to the description, since I can't explicitly say required: [] at the top level :/

next_batch:
type: string
description: |-
A pagination token for the response, if there are any more results.

This comment has been minimized.

@richvdh

richvdh Sep 29, 2016

Member

let's be more explicit here: a client determines that they have reached the end of the list by the absence of next_batch ?

@richvdh

This comment has been minimized.

Member

richvdh commented Sep 29, 2016

Also: needs a changelog entry, please.

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Sep 29, 2016

@richvdh PTAL

@richvdh

sorry, still fonxing this on the null fields situation

A pagination token for the response.
A pagination token that allows fetching previous results. The
absence of this token means there are no results before this
batch, i.e. this is the first batch.

This comment has been minimized.

@richvdh

richvdh Sep 30, 2016

Member

Ah; is the direction of pagination implied in the token, unlike other APIs where there is an explicit direction flag? might be worth emphasising that.

This comment has been minimized.

@erikjohnston

erikjohnston Sep 30, 2016

Member

I've updated to mention this.

@@ -35,6 +35,8 @@
- Add top-level ``account_data`` key to the responses to ``GET /sync`` and
``GET /initialSync``
(`#380 <https://github.com/matrix-org/matrix-doc/pull/380>`_).
- Add pagination and filter support to ``/publicRooms``

This comment has been minimized.

@richvdh

richvdh Sep 30, 2016

Member

... and more details in the response?

This comment has been minimized.

@erikjohnston

erikjohnston Sep 30, 2016

Member

Added more details

@richvdh richvdh assigned erikjohnston and unassigned richvdh Sep 30, 2016

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Sep 30, 2016

name: since
type: string
description: |-
A pagination token from a previous request, allowing clients to

This comment has been minimized.

@dbkr

dbkr Oct 3, 2016

Member

Maybe worth clarifying that this can be either a prev or next token?

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

(done)

type: string
description: |-
A pagination token from a previous request, allowing clients to
get the next batch of rooms.

This comment has been minimized.

@dbkr

dbkr Oct 3, 2016

Member

or previous

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

or previous

@dbkr dbkr removed their assignment Oct 3, 2016

}
400:
description: >
The request body is malformed or the room alias specified is already taken.

This comment has been minimized.

@richvdh

richvdh Oct 3, 2016

Member

This doesn't sound right. There is no request body for a GET, and there is no specified room alias.

Similarly on the POST endpoint.

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

(done)

400:
description: >
The request body is malformed or the room alias specified is already taken.
post:

This comment has been minimized.

@richvdh

richvdh Oct 3, 2016

Member

GET endpoint is missing tags

A pagination token that allows fetching previous results. The
absence of this token means there are no results before this
batch, i.e. this is the first batch.
The direction of pagination is specified solely by which token

This comment has been minimized.

@richvdh

richvdh Oct 3, 2016

Member

I think this would make more sense on the since parameter than here.

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

(done)

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

Not done. Needs doing on the POST endpoint too.

type: object
title: "Filter"
description: |-
Optional filtering of the returned rooms.

This comment has been minimized.

@richvdh

richvdh Oct 3, 2016

Member

I think "Filter to apply to the results" reads better. No need to emphasise that it is optional (particularly since you don't for since and limit).

@richvdh richvdh assigned erikjohnston and unassigned richvdh Oct 3, 2016

@erikjohnston erikjohnston removed their assignment Oct 11, 2016

type: number
description: |-
Limit the number of results returned, ordered by number of
memebers in the room. Defaults to no limit.

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

"memebers"

type: string
description: |-
A pagination token from a previous request, allowing clients to
get the next batch of rooms.

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

or previous

type: number
description: |-
Limit the number of results returned, ordered by number of
memebers in the room. Defaults to no limit.

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

"memebers"

name: limit
type: number
description: |-
Limit the number of results returned, ordered by number of

This comment has been minimized.

@richvdh

richvdh Oct 11, 2016

Member

Do we return the busiest or least busy rooms? I'm assuming the former, but be explicit please.

Joined members? Invited members? Banned/left members?

Are the results returned ordered by number of members, or does this just apply to the limit? If the former, this should go elsewhere. If the latter, you probably want something like "Limit for the number of results returned. If there are more results, the rooms with the {most|fewest} members will be returned. By default, there is no limit." (You probably want that anyway).

Ditto for the POST API.

@richvdh richvdh assigned erikjohnston and unassigned richvdh Oct 11, 2016

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Nov 21, 2016

richvdh added some commits Nov 21, 2016

Final public_rooms clarifications
 * order by *joined* members
 * clarify pagination direction behaviour
@dbkr

dbkr approved these changes Nov 21, 2016

@richvdh richvdh merged commit 02a8715 into master Nov 21, 2016

2 checks passed

Docs (Commit) Build #1954 succeeded in 27 sec
Details
Docs (Merged PR) Build finished.
Details

@richvdh richvdh referenced this pull request Aug 31, 2017

Closed

Spec POST /publicRooms API #868

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment