initial cut at a room summary API

[ara4n]

this works, although considers all room membership events rather than just joined
ones as i'm failing to see a nice way to filter based on membership.

Implements matrix-org/matrix-spec-proposals#688

Builds on #3568 (in turn #3567 (in turn #3331 (in turn #2970))). (all now merged)

Sytest at matrix-org/sytest#470

[bwindels]

Should be m.heros? I get a exceptions.KeyError: 'heros' when running this locally.

[bwindels]

Fixed it, as am 99% certain that that is what it should be, and it unblocked me locally from syncing.

[bwindels]

m.joined_member_count seems to be irrespective of membership. While testing in a DM, upon the other user leaving, I get this from /sync for the given room. Is it also intended that the left user is still included in m.heros? I guess it could, as you won't want the room name to change, but different from what the proposal says:

{
  "!VHGOLZdMXhOiFmizWb:localhost": {
    "timeline": {
      "limited": false,
      "prev_batch": "s500_5160_4_83_232_1_1_56_1",
      "events": [
        {
          "origin_server_ts": 1532351680209,
          "sender": "@bruno5:localhost",
          "event_id": "$153235168010hSmVV:localhost",
          "unsigned": {
            "prev_content": {
              "membership": "join",
              "avatar_url": "mxc://localhost/JyLQUEVWImjlOIyJRLGxqZtD",
              "displayname": "Bruno Mars"
            },
            "prev_sender": "@bruno5:localhost",
            "replaces_state": "$15323515718rGSwF:localhost",
            "age": 81
          },
          "state_key": "@bruno5:localhost",
          "content": {
            "membership": "leave"
          },
          "membership": "leave",
          "type": "m.room.member"
        }
      ]
    },
    "summary": {
      "m.joined_member_count": 2,
      "m.heros": [
        "@bruno1:localhost",
        "@bruno5:localhost"
      ]
    }
  }
}

[ara4n]

Yup, see #3574 (comment). There are a bunch of correctness errors here, but at least the API can be developed against even if the data it gives is a bit wrong currently. See also the other FIXMEs at https://github.com/matrix-org/synapse/pull/3574/files#diff-6fee1701e980dbc9da57a5a108ee7b65R556 for other limitations right now.

[bwindels]

Yeah, just realized you mentioned this in the description

[ara4n]

@richvdh PTAL.

[richvdh]

lgtm modulo nitpicking about early returns.

Note that the PR is currently based on #3568 which has pending comments.

[richvdh]

can we do an early return if name_id or canonical_alias_id rather than having this massive if block?

[richvdh]

Again, make this into an early return?

[ara4n]

I'm very dubious about the early returns; it feels much less error prone to say "only do this thing if this condition is true" rather than "return early if this condition is false, as coincidentally the remainder of the method is handling that condition being true" - as:

• the latter results in condition-specific stuff being grouped together (indentationwise) with the generic bits of the method
• it's harder to read, as the reader has to skim for early returns
• if folks need to add more functionality to the method, they can no longer do it at the end, as the end has ended up on some obscure conditional branch (which may not be obvious unless they skim the whole thing for early returns)
• it feels like trying to reduce nesting for the sake of reducing nesting.

However, I've gone and done it anyway in a bid to get this merged, but i am still wondering whether we are making reviews too subjective (or making it too hard to determine the 'strength' of review feedback - and whether it's a "if i were doing this, i'd have considered doing it like this, but it's pretty subjective" or a "this is cosmetic nitpick but something that Should Be Fixed.")

[ara4n]

@richvdh ptal

[richvdh]

I already LGTMed this; it now looks even better. Just waiting for the tests to run.

To address your points, certainly a better alternative might be to split out separate functions.

the latter results in condition-specific stuff being grouped together (indentationwise) with the generic bits of the method

yes it does; I don't really see that as a problem.

it's harder to read, as the reader has to skim for early returns

Personally I find early returns much easier to read than having to scroll up and down to figure out how the indentation matches up and which "if" clause we're leaving.

if folks need to add more functionality to the method, they can no longer do it at the end, as the end has ended up on some obscure conditional branch (which may not be obvious unless they skim the whole thing for early returns)

I think adding stuff to the end of a function without looking at what else is going on would be, er, foolhardy.

it feels like trying to reduce nesting for the sake of reducing nesting.

sure it is.

i am still wondering whether we are making reviews too subjective (or making it too hard to determine the 'strength' of review feedback - and whether it's a "if i were doing this, i'd have considered doing it like this, but it's pretty subjective" or a "this is cosmetic nitpick but something that Should Be Fixed.")

well, we discussed this in the team meeting the other day, and yes, these topics were discussed. I don't think here is the right place to reiterate that conversation, but in this case: I am coming to this as the person who is probably going to have to maintain this code (or at least review future changes to it in the future), and I do not want to have to maintain code that has massive nested if blocks that are hard to follow. Thank you for changing it.

[richvdh]

nit: in future, please do add docstrings to public functions: in particular it's important to document the args and return types.