Add support for "m.federate" and room name in createRoom API method #245

slipeer · 2018-07-12T07:37:04Z

This PR adds:

Ability to set a parameter 'm.federate' when creating a room
Ability to set name when creating a room
Test for createRooom api method

Half-Shot

Not sure why the .travis.yml got changed, if it was causing grief then I doubt sudo is going to help.

Generally speaking the code is good but the tests need some work.

Half-Shot · 2018-07-12T10:31:24Z

.travis.yml

@@ -1,4 +1,4 @@
-sudo: false
+sudo: required


Why has this been changed?

without it travis was fail at olm make install - check travis build history.
I can revert but it will brake travis build.

I see that now. Don't put sudo in here anyway, this needs fixing by the maintainer.

Ok. Now I'll take it away.

Half-Shot · 2018-07-12T10:34:05Z

matrix_client/api.py

+                name=None,
+                is_public=False,
+                invitees=None,
+                federate=None


I don't like leaving booleans as None. You should set this to true, so that users of the API know that federation is on by default. I'd actually argue None is closer to False, which is obviously dangerously wrong.

https://matrix.org/docs/spec/client_server/unstable.html#m-room-create

On my homeservet it's off by default...
I prefer to give a choice of three options: explicitly turn on, explicitly turn off and leave by default.

The default given in the spec is True. If your homeserver does something different then it's non-conforming?

We discuss about SDK implementation or about settings on my homeserver? I gave an example to show that the behavior of the server is not always obvious and it is useful to give the developer the opportunity to explicitly set this parameter.

We should specify the matrix spec default in the methods docstring. None is fine as default kwarg; we just need to be explicit that according to matrix spec this defaults to True in the docstring. If servers have a setting that can change the default value of this and that's considered part of the spec, it's a spec bug that the setting isn't mentioned at the place @Half-Shot linked.

So based on a conversation with @ara4n in #matrix-spec:matrix.org, a server that had a default value other than True for room federation would be non-compliant with the matrix spec. I'm fine with including this distinction between None and False if it's useful to you, but the docstring for this method needs to explicitly say that True is the default.

https://matrix.to/#/!YkZelGRiqijtzXZODa:matrix.org/$1531495739346932GaxTh:matrix.org

Half-Shot · 2018-07-12T10:34:47Z

matrix_client/api.py

            content["invite"] = invitees
+        if name:
+            content["name"] = name
+        if federate is not None:


wrt above comment: if federate is False

feredate can be True, False or None. If it's None we do not add it to json.

Half-Shot · 2018-07-12T10:38:16Z

test/api_test.py

+        self.cli.api.create_room(
+            name="test",
+            alias="#test:example.com",
+            is_public=True,


Please don't combine two tests here. If you are also trying to test is_public, create two new tests toggling that behavior.

Half-Shot · 2018-07-12T10:38:45Z

test/api_test.py

+            federate=True
+        )
+        self.cli.api.create_room(name="test3")
+        req = responses.calls[0].request


Half-Shot · 2018-07-12T10:40:11Z

test/api_test.py

+    room_id = "#foo:matrix.org"
+
+    @responses.activate
+    def test_create_room(self):


I'd argue we should be splitting up tests if you are testing different behaviours. E.g. test_create_room_federate, test_create_room_visibility

slipeer · 2018-07-12T11:13:50Z

@Half-Shot That's better?

Half-Shot · 2018-07-12T12:15:30Z

Much better, thank you 😄 .

Regarding the travis issues..I need to find out the situation from @non-Jedi.

Regarding the True/False/None issue, I guess you have a valid point if your homeserver does something different.

I'm approving the changes but I want to see if we can fix the travis issues first.

non-Jedi · 2018-07-13T15:15:34Z

matrix_client/api.py

+            name (str): Optional. Name for neq room.
            is_public (bool): Optional. The public/private visibility.
            invitees (list<str>): Optional. The list of user IDs to invite.
+            federate (bool): Optional. Сan a room be deferated.


non-Jedi · 2018-07-13T15:45:44Z

In addition to the docstring update, could you please also rebase this on master (so Travis runs successfully) and squash down some of the extra commits?

non-Jedi · 2018-07-13T15:46:25Z

matrix_client/api.py


        Args:
            alias (str): Optional. The room alias name to set for this room.
+            name (str): Optional. Name for neq room.


fix matrix-org#242

slipeer · 2018-07-16T05:23:36Z

@non-Jedi Now everything is correct?

non-Jedi · 2018-07-16T13:42:53Z

matrix_client/api.py

+            name (str): Optional. Name for new room.
            is_public (bool): Optional. The public/private visibility.
            invitees (list<str>): Optional. The list of user IDs to invite.
+            federate (bool): Optional. Сan a room be federated.


Please include that this defaults to True.

I already explained for Half-Shot why I left three options:
#245 (comment)

@non-Jedi included.

Sorry to be anal about this, but the spec says that homeservers default to true on that parameter if it isn't included. It's not a synapse-specific behavior, and we need to document the behavior of a spec-compliant HS. So something like: "Defaults to True if not specified" is what I'm after here.

That's better?

Yep. That's fine. Thanks @slipeer. I'll merge when I'm at a computer I've set up with ssh to my github profile.

non-Jedi · 2018-07-16T13:43:57Z

.travis.yml

@@ -1,4 +1,4 @@
-sudo: required
+sudo: false


Please remove this change from the commit.

…eateRoom Added test for createRoom api method

non-Jedi · 2018-07-18T01:50:51Z

Forgot to ask for this sooner. Sorry. Could you signoff per the CONTRIBUTING.rst please?

https://github.com/matrix-org/matrix-python-sdk/blob/master/CONTRIBUTING.rst

Will have to do for each PR before I can merge. Simplest way is to just signoff in a comment.

slipeer · 2018-07-18T04:11:13Z

Signed-off-by: Pavel Kardash slipeer@gmail.com

non-Jedi · 2018-07-29T00:48:14Z

Cherry-picked into master. Thanks!

slipeer force-pushed the mfederate_support branch from 751f29c to d9d4970 Compare July 12, 2018 10:17

Half-Shot requested changes Jul 12, 2018

View reviewed changes

Half-Shot approved these changes Jul 12, 2018

View reviewed changes

Half-Shot assigned Half-Shot and non-Jedi and unassigned Half-Shot Jul 13, 2018

non-Jedi reviewed Jul 13, 2018

View reviewed changes

Zil0 added 13 commits July 16, 2018 08:19

upload identity keys

2a5106c

upload one-time keys

87da7c7

add upload identity/one-time keys tests

338e46d

plug-in encryption in client

bd3ad37

allow encryption configuration from client

077e538

use class docstrings instead of __init__ docstrings

0b83a11

fix matrix-org#242

enable encryption in a room

68ece48

handle m.room.encryption

c953681

add get_state_event API method

f097ee9

automatically check if a room is encrypted

ee77982

track one time keys count in sync response

f98937f

add one-time key counts tracking tests

d30e8e5

fix bug when detecting encryption in _mkroom

bfd65d8

slipeer force-pushed the mfederate_support branch from 38f0764 to 4a5d698 Compare July 16, 2018 05:20

non-Jedi reviewed Jul 16, 2018

View reviewed changes

.travis.yml Outdated

@@ -1,4 +1,4 @@

sudo: required

sudo: false

Copy link

Collaborator

non-Jedi Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this change from the commit.

slipeer force-pushed the mfederate_support branch 2 times, most recently from 5d6aaa5 to 0ba73e2 Compare July 16, 2018 14:49

Added support for parameters 'name' and 'federation' in API method cr…

11bf397

…eateRoom Added test for createRoom api method

slipeer force-pushed the mfederate_support branch from 0ba73e2 to 11bf397 Compare July 16, 2018 15:25

non-Jedi closed this Jul 29, 2018

Add support for "m.federate" and room name in createRoom API method #245

Add support for "m.federate" and room name in createRoom API method #245

Uh oh!

Conversation

slipeer commented Jul 12, 2018

Uh oh!

Half-Shot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non-Jedi Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slipeer commented Jul 12, 2018

Uh oh!

Half-Shot commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non-Jedi commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slipeer commented Jul 16, 2018

Uh oh!

non-Jedi Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non-Jedi commented Jul 18, 2018

Uh oh!

slipeer commented Jul 18, 2018

Uh oh!

non-Jedi commented Jul 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

non-Jedi Jul 13, 2018 •

edited

Loading

Half-Shot commented Jul 12, 2018 •

edited

Loading

non-Jedi Jul 16, 2018 •

edited

Loading