Add query_params to appropriate methods #143

Cadair · 2017-09-08T14:37:44Z

This now adds **query_params to most methods to allow specification of appservice parameters like ts and user_id.

It also does a bunch of documentation updates and moves from sphinxcontrib.napoleon to sphinx.ext.napoleon.

This introduces a breaking change of timestamp= to ts=.

Signed-off-by: Stuart Mumford stuart@cadair.com

matrixbot · 2017-09-08T14:37:48Z

Can one of the admins verify this patch?

non-Jedi · 2017-09-09T00:51:38Z

Thanks @Cadair. A couple notes:

Please update the docstrings of any methods whose arguments you change to
reflect the new arguments even if those arguments are strictly optional.
The more I think about this, the less I like having user_id as a
separate kwarg on each method. We should just put a query_params kwarg
on each method instead. This would also get rid of all the timestamp
kwargs littered everywhere. This gives at least 2 benefits:
1. The method signatures more closely match the api (information going in
  the headers is separate from information going in query_params is
  separate from the json payload... mostly).
2. Any future (optional) extensions to the CS api involving query_params
  are already accounted for.
We should include a note in each docstring listing possible contents of
the query_params kwarg (e.g. "ts" and "user_id").

non-Jedi · 2017-09-10T00:47:25Z

Can't comment on the sphinx stuff since I'm not familiar with it at all, but I can tell you @Half-Shot will want it in a separate PR. I can also tell you it will almost certainly be very appreciated. ;)

Oh, also, you'll need to sign-off before anything is merged. See Contributing.rst

Cadair · 2017-09-10T15:24:58Z

@non-Jedi This now only contains changes relevant to the title, the other doc fixes are in #144.

non-Jedi

I strongly prefer using an explicit query_params={} rather than **query_params since we already use **kwargs on login.

non-Jedi · 2017-09-10T20:26:52Z

matrix_client/api.py

    """

-    def __init__(self, base_url, token=None, identity=None):
+    def __init__(self, base_url, token=None):


I'd like to keep the ability to use an api object per mxid without having to explicitly pass in the user_id kwarg on every request.

non-Jedi · 2017-09-10T20:28:25Z

matrix_client/api.py

+                Common values for ``query_params`` are:
+
+                | ts: timestamp for event
+                | user_id: user id for transaction.


This is good documentation. Need to add note in parentheses about these values only being useful for application services. Matrix newcomers may use python sdk as reference without reading the spec.

non-Jedi · 2017-09-10T20:38:06Z

matrix_client/api.py

-        return self._send("GET", "/initialSync", query_params={"limit": limit})
+        query_params['limit'] = limit
+        return self._send("GET", "/initialSync",
+                          query_params=query_params)


If we're going with the convention of query_params being slurped by
**query_params, we should also consider slurping the query_params for _send
in the same way and splatting the query_params dict here into the _send
call.

non-Jedi · 2017-09-10T20:38:52Z

matrix_client/api.py

+                Common values for ``query_params`` are:
+
+                | ts: timestamp for event
+                | user_id: user id for transaction.


Same note as above for all documentation about params being AS-specific.

non-Jedi · 2017-09-10T20:41:26Z

matrix_client/api.py

-        request = {
-            "timeout": timeout_ms
-        }
+        request = query_params


We should rename this variable query_params for consistency.

non-Jedi · 2017-09-10T20:55:34Z

matrix_client/api.py


    def sync(self, since=None, timeout_ms=30000, filter=None,
-             full_state=None, set_presence=None):
+             full_state=None, set_presence=None, **query_params):


For consistency of the api, we should probably deprecate since, timeout_ms, filter, full_state, and set_presence kwargs in favor of slurping those things with **query_params. Unfortunately, I don't think there's any way to raise a deprecation warning for kwargs specified positionally and not raise a warning for the same kwargs specified by identifier. :/

I think you could do that by using a decorator that does funky things with signature objects. I think it might be Python 3 only though.

non-Jedi · 2017-09-10T20:57:04Z

matrix_client/api.py

+            **query_params: Extra parameters to be sent in the HTTP request.
+                Common values for ``query_params`` are:
+
+                | ts: timestamp for event


My gut says that ts is not a query_param that makes sense for this api call.

non-Jedi · 2017-09-10T20:59:04Z

matrix_client/api.py

+            **query_params: Extra parameters to be sent in the HTTP request.
+                Common values for ``query_params`` are:

+                | ts: timestamp for event


Again, we need to make sure query_param values make sense for the api call before mentioning them in the documentation. user_id does, but ts probably doesn't.

non-Jedi · 2017-09-10T21:07:27Z

matrix_client/api.py

+            query_params=query_params
        )

    def login(self, login_type, **kwargs):


This is why I don't like using **query_params. Now we've introduced
inconsistency to our api methods. Sometimes, extra kwargs are treated as extra
json, and sometimes they're params.

Additionally, there might be times when an AS that's e2e-aware would make use of
login/logout api methods. Not sure; would need to double-check this.

non-Jedi · 2017-09-10T21:10:25Z

matrix_client/api.py

+            "timeout": timeout,
+            "from": from_token
+        }
+        query_params.update(params)


query_params["timeout"] = timeout query_params["from"] = from_token

is slightly cleaner, but whatever.

This makes it easy to have a appservice use one API instance to act as many users.

Cadair · 2018-01-03T08:57:45Z

@non-Jedi I have changed from **query_params to query_params=None and addressed all your other relevant comments I think.

non-Jedi · 2018-01-03T14:19:43Z

@Cadair thanks. Will take a look when I get a chance (probably after I finally get to the doc updates). Could you fix the failing flake8 test from Travis, please?

non-Jedi · 2018-01-05T15:14:33Z

@Cadair turns out query_params={} was terrible advice on my part. Having
mutable default args in python doesn't produce the expected result. For example:

>>> def f(a=[]):
...     a.append(None)
...     return a
... 
>>> f()
[None]
>>> f()
[None, None]
>>> f()
[None, None, None]

Haven't thought about this enough to know what correct solution is, but I just
remembered the thing about mutable default kwargs, and thought I should note it
here. Sorry about that.

Cadair · 2018-01-06T09:49:52Z

Yeah I thought about that the other day as well. It's not just query params that does that in the api though. I will go through and replace them all with None and add conditionals to change them to dicts where needed.

non-Jedi · 2018-05-14T18:09:14Z

Thinking about this again, I think AS support should be pushed entirely into a subclass of MatrixHttpApi that defines a custom _send. Then each public method of MatrixHttpApi would have an extras kwarg which would be forwarded unmodified to _send. For the AS subclass of MatrixHttpApi, _send would look into extras (make it a dict I suppose) for the AS-specific params. Thoughts?

I'm trying to push this sdk towards being as modular/composable as possible, and this seems like a step in the correct direction while also making things unique to different _send backends explicit rather than implicit.

Cadair · 2018-05-14T18:17:55Z

I need to give this some thought. off the top of my head I feel like this is a good argument for **kwargs to get passed to _send._

non-Jedi · 2018-05-14T18:22:55Z

Could be. Need to look again at api methods that currently slurp kwargs first. If we do go that route, we should call the slurped kwargs **extras or something evocative of their role (but more generic than **query_params) rather than **kwargs. I'm gonna close this one for now and open a new issue since the approach here I think isn't quite right no matter what direction we end up going.

Cadair changed the title ~~Add an explicit user_id kwarg everywhere~~ Add **query_params to appropriate methods Sep 9, 2017

Cadair mentioned this pull request Sep 10, 2017

Documentation updating #144

Merged

Cadair force-pushed the user_id2 branch from 7e374e1 to 50719a1 Compare September 10, 2017 15:22

non-Jedi requested changes Sep 10, 2017

View reviewed changes

non-Jedi added the Awaiting Changes label Sep 22, 2017

Cadair added 8 commits January 3, 2018 07:54

Add an explicit user_id kwarg everywhere

72b34ea

This makes it easy to have a appservice use one API instance to act as many users.

Change from user_id to **query_params

4b04f92

Remove timestamp and add more query_params documentation

121d8ca

document **query_params

e670e7c

make timeout always an int

a621c7c

add back identity

dca7930

rename request to query_params

372fb11

replace **query_params with query_params=None

037c5f1

Cadair force-pushed the user_id2 branch from 50719a1 to b5cba5f Compare January 3, 2018 08:56

small tidy

b5cba5f

change to query_params={}

97bb674

Cadair closed this Jan 3, 2018

Cadair reopened this Jan 3, 2018

Cadair changed the title ~~Add **query_params to appropriate methods~~ Add query_params to appropriate methods Jan 3, 2018

Cadair added 2 commits January 3, 2018 15:50

fix typo

909d977

more query_params

e4b3230

even more query_params

3b5af6c

non-Jedi closed this May 14, 2018

non-Jedi mentioned this pull request May 14, 2018

Move features of MatrixHttpApi related to application-services into subclass #204

Open

1 task

Add query_params to appropriate methods #143

Add query_params to appropriate methods #143

Uh oh!

Conversation

Cadair commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matrixbot commented Sep 8, 2017

Uh oh!

non-Jedi commented Sep 9, 2017

Uh oh!

non-Jedi commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cadair commented Sep 10, 2017

Uh oh!

non-Jedi 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cadair commented Jan 3, 2018

Uh oh!

non-Jedi commented Jan 3, 2018

Uh oh!

non-Jedi commented Jan 5, 2018

Uh oh!

Cadair commented Jan 6, 2018

Uh oh!

non-Jedi commented May 14, 2018

Uh oh!

Cadair commented May 14, 2018

Uh oh!

non-Jedi commented May 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Cadair commented Sep 8, 2017 •

edited

Loading

non-Jedi commented Sep 10, 2017 •

edited

Loading