Skip to content
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

Python 3 support #9

Merged
merged 21 commits into from
Mar 19, 2016
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Matrix Client SDK for Python
:target: https://pypi.python.org/pypi/matrix-client/
:alt: Latest Version

This is a Matrix client-server SDK for Python 2.x.
This is a Matrix client-server SDK for Python 2.x and 3.x.

Usage
=====
Expand Down Expand Up @@ -49,3 +49,12 @@ transaction ID (``txn_id``) which will be incremented for each request.
Client
------
This encapsulates the API module and provides object models such as ``Room``.

Samples
=======
A collection of samples are included, written in Python 3.

To start a sample without having to install the sdk run:

startSample.sh samplefilename.py
Copy link
Member

Choose a reason for hiding this comment

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

Please update this :)


47 changes: 40 additions & 7 deletions matrix_client/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
import json
import re
import requests
import urllib

try:
import urlparse
from urllib import quote
except ImportError:
from urllib.parse import quote
import urllib.parse as urlparse # For python 3

class MatrixError(Exception):
Expand Down Expand Up @@ -130,7 +132,7 @@ def join_room(self, room_id_or_alias):
if not room_id_or_alias:
raise MatrixError("No alias or room ID to join.")

path = "/join/%s" % urllib.quote(room_id_or_alias)
path = "/join/%s" % quote(room_id_or_alias)

return self._send("POST", path)

Expand Down Expand Up @@ -159,10 +161,10 @@ def send_state_event(self, room_id, event_type, content, state_key=""):
state_key(str): Optional. The state key for the event.
"""
path = ("/rooms/%s/state/%s" %
(urllib.quote(room_id), urllib.quote(event_type))
(urlparse.quote(room_id), urlparse.quote(event_type))
)
if state_key:
path += "/%s" % (urllib.quote(state_key))
path += "/%s" % (quote(state_key))
return self._send("PUT", path, content)

def send_message_event(self, room_id, event_type, content, txn_id=None):
Expand All @@ -180,11 +182,24 @@ def send_message_event(self, room_id, event_type, content, txn_id=None):
self.txn_id = self.txn_id + 1

path = ("/rooms/%s/send/%s/%s" %
(urllib.quote(room_id), urllib.quote(event_type),
urllib.quote(unicode(txn_id)))
(quote(room_id), quote(event_type), quote(str(txn_id)))
)
return self._send("PUT", path, content)

# content_type can be a image,audio or video
# extra information should be supplied, see https://matrix.org/docs/spec/r0.0.1/client_server.html
def send_content(self,room_id, item_url,item_name,item_type,extra_information=None):
Copy link
Member

Choose a reason for hiding this comment

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

Spaces please :)

Copy link
Member

Choose a reason for hiding this comment

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

And throughout (e.g. :201)

if extra_information == None:
extra_information = {}

content_pack = {
"url":item_url,
"msgtype":"m."+item_type,
Copy link
Member

Choose a reason for hiding this comment

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

Please can we drop the m. auto-filling and rely on the caller to give the right msgtype. They need to know the API fields anyway, so transforming msgtype into item_type doesn't really make anything clearer, and makes it impossible for people to use non-standard msgtypes (e.g. of the form org.matrix.custom.msgtype).

"body":item_name,
"info":extra_information
}
Copy link
Member

Choose a reason for hiding this comment

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

Please put spaces after :

return self.send_message_event(room_id,"m.room.message",content_pack)

def send_message(self, room_id, text_content, msgtype="m.text"):
"""Perform /rooms/$room_id/send/m.room.message

Expand Down Expand Up @@ -302,7 +317,7 @@ def get_emote_body(self, text):
"body": text
}

def _send(self, method, path, content=None, query_params={}, headers={}):
def _send(self, method, path, content=None, query_params={}, headers={},content_type="application/json"):
method = method.upper()
if method not in ["GET", "PUT", "DELETE", "POST"]:
raise MatrixError("Unsupported HTTP method: %s" % method)
Expand All @@ -323,3 +338,21 @@ def _send(self, method, path, content=None, query_params={}, headers={}):
)

return response.json()

def media_upload(self,content,content_type):
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer it if this was factored out along with _send and friends, as there is so much duplication between the two. The only notable difference AFAICS is the base endpoint URL (no reason why we can't make this an arg with self.url as the default) and whether or not it is JSON (which determines if we json.dumps() the content).

query_params = {"access_token":self.token}
headers = {"Content-Type":content_type}
endpoint = self.url.replace( "/_matrix/client/api/v1","/_matrix/media/r0/upload")

response = requests.request(
"POST", endpoint, params=query_params,
data=content, headers=headers
, verify=self.validate_cert #if you want to use SSL without verifying the Cert
)

if response.status_code < 200 or response.status_code >= 300:
raise MatrixRequestError(
code=response.status_code, content=response.text
)

return response.json()
14 changes: 13 additions & 1 deletion matrix_client/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ def start_listener_thread(self, timeout=30000):
e = sys.exc_info()[0]
print("Error: unable to start thread. " + str(e))

def upload(self,content,content_type):
response = self.api.media_upload(content,content_type)["content_uri"]
Copy link
Member

Choose a reason for hiding this comment

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

If content_uri doesn't exist for whatever reason this will not fail gracefully (with a nasty KeyError rather than with MatrixRequestError or some other well-defined exception). We should really be aiming to meet the terms of our API contract regardless of what stuff is thrown at us by the homeserver.

return response

def _mkroom(self, room_id):
self.rooms[room_id] = Room(self, room_id)
return self.rooms[room_id]
Expand Down Expand Up @@ -171,6 +175,15 @@ def send_text(self, text):
def send_emote(self, text):
return self.client.api.send_emote(self.room_id, text)

def send_image(self,url,name,size,mimetype,width=500,height=500):
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have defaults for width/height. Better to omit non-required information than to give false information here.

extra_info = {
'size':size,
'mimetype':mimetype,
'width':width,
'height':height
}
return self.client.api.send_content(self.room_id, url,name,"image",extra_info)
Copy link
Member

Choose a reason for hiding this comment

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

I would be tempted to kwarg these for clarity, rather than expecting people to remember the ordering of a 5 arg function in another file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Since it's the same naming as the media API, this might be a better plan.


def add_listener(self, callback):
self.listeners.append(callback)

Expand Down Expand Up @@ -256,4 +269,3 @@ def update_aliases(self):
return False
except MatrixRequestError:
return False

75 changes: 75 additions & 0 deletions samples/SimpleChatClient.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#!/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work on Ubuntu 15.10 - It requires /usr/bin/env

from matrix_client.client import MatrixClient
from matrix_client.api import MatrixRequestError
from requests.exceptions import MissingSchema

from getpass import getpass

import sys

def on_message(event):
if event['type'] == "m.room.member":
if event['membership'] == "join":
print("{0} joined".format(event['content']['displayname']))
elif event['type'] == "m.room.message":
if event['content']['msgtype'] == "m.text":
print("{0}: {1}".format(event['sender'],event['content']['body']))
else:
print(event['type'])

host = "";
username = "";
room = "";
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Semicolons 😄


if len(sys.argv) > 1:
host = sys.argv[1]
else:
host = input("Host (ex: http://localhost:8008 ): ")

client = MatrixClient(host)

if len(sys.argv) > 2:
username = sys.argv[2]
else:
username = input("Username: ")

password = getpass() #Hide user input

try:
client.login_with_password(username,password)
except MatrixRequestError as e:
if e.code == 403:
print("Bad username or password.")
else:
print("Check your sever details are correct.")
sys.exit(e.code)
except MissingSchema:
print("Bad URL format.")
sys.exit(1)

room = None
if len(sys.argv) > 3:
room = sys.argv[3]
else:
room = input("Room ID/Alias: ")

try:
room = client.join_room(room)
except MatrixRequestError as e:
if e.code == 400:
print("Room ID/Alias in the wrong format")
else:
print("Couldn't find room.")
Copy link
Member

Choose a reason for hiding this comment

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

Worth splatting out the entire error here (at least the M_ code and the HTTP status) to aid debugging.

Copy link
Member

Choose a reason for hiding this comment

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

And throughout (e.g. :44)

sys.exit(2)
Copy link
Member

Choose a reason for hiding this comment

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

You either need to document the significance of these error codes or you're just being inconsistent. It looks like client-side errors give 1 but server-side errors give 2, except in :45 which tries to exit with the HTTP error code.


room.add_listener(on_message)
client.start_listener_thread()


shouldRun = True
Copy link
Member

Choose a reason for hiding this comment

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

please_use_underscores_in_python

Copy link
Member

Choose a reason for hiding this comment

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

Equally, you could just while true: and break which removes the need for this variable entirely, but I don't mind.

while shouldRun:
msg = input()
if msg == "/quit":
shouldRun = False
else:
room.send_text(msg)
11 changes: 11 additions & 0 deletions samples/startSample.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/sh

if [ "$1" == "-h" ] || [ $# -lt 1 ]; then
echo "Usage: `basename $0` scriptname [OPTIONS] "
exit 0
fi

# Will run a sample without requiring an install of the library.
PYTHONPATH="$(dirname $( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd ))"
export PYTHONPATH=$PYTHONPATH
python "$@"
Copy link
Member

Choose a reason for hiding this comment

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

Naively running this on Ubuntu 15.10 produces:

$ ./startSample.sh SimpleChatClient.py 
./startSample.sh: 3: [: SimpleChatClient.py: unexpected operator
./startSample.sh: 9: ./startSample.sh: Bad substitution

Copy link
Member

Choose a reason for hiding this comment

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

I don't like how unix-specific this is. I also don't like the idea of requiring a bootstrapping shell script in general. Can you please just dynamically alter the PYTHONPATH like so:

import sys
sys.path.insert(0, "../") # add ../ to PYTHONPATH
from matrix_client.client import MatrixClient
from matrix_client.api import MatrixRequestError

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was unaware you could edit it while running, but I don't see why not 👍