Push notifications #101
Push notifications #101
Conversation
|
@jdavid can you please take a look at the failing tests? |
|
Yes I will look. Actually the failure also happens in the master branch, it's unrelated to the PR. |
|
@jdavid Then it's probably better to solve it a separate PR, and then rebase this when the fix for tests has been merged. |
ably/rest/push.py
Outdated
| def ably(self): | ||
| return self.__ably | ||
|
|
||
| def publish(self, recipient, data): |
There was a problem hiding this comment.
I'm comparing with the analogous RET publish method (https://github.com/ably/ably-python/blob/master/ably/rest/channel.py#L70)
There, we have inline doc strings - should we also do that here?
ably/rest/push.py
Outdated
|
|
||
| body = data.copy() | ||
| body.update({'recipient': recipient}) | ||
| return self.ably.http.post('/push/publish', body=body) |
There was a problem hiding this comment.
Elsewhere there's a timeout argument in the http post - should we also have that here?
| if protocol == 'json': | ||
| self.assertEquals(response.headers['content-type'], 'application/json') | ||
| json.loads(response.text) | ||
| if response.content: |
There was a problem hiding this comment.
Not a super expert in Python, but why not testing for response.text, which is what you access in the subsequent line?
There was a problem hiding this comment.
Yes, I copied if response.content: from the msgpack case, just a few lines below.
content is the response body as a byte string, text is the same as a text string (unicode)
I have done a different fix, and used response.json() instead of json.loads(...).
DevicePushTransportType, DevicePlatform and DeviceFormFactor
|
I would prefer please if each set of changes had its own PR, raised against this @funkyboy can you help to set that up please? |
|
Okay, closing this PR. Will reset the |
|
See #106 |
Uh oh!
There was an error while loading. Please reload this page.