[Merge] lp:~flo-fuchs/mailman/restclient into lp:mailman
Barry Warsaw
barry at canonical.com
Tue Jun 29 20:45:40 CEST 2010
Thanks for getting this client library started, Florian! Here are some
thoughts on your merge proposal.
As we discussed on IRC, I now think the best place for this code is in
src/mailmanclient. Don't worry about that though; I'll fix that up when I
merge to trunk. I know for now it's difficult getting the test suite to run
with that layout, so I'll take that on. I don't think it will be too
difficult, but there will also be some other useful helpers to share.
Omitting the non-controversial stuff.
=== added file 'src/mailman/rest/docs/restclient.txt'
--- src/mailman/rest/docs/restclient.txt 1970-01-01 00:00:00 +0000
+++ src/mailman/rest/docs/restclient.txt 2010-06-25 16:50:42 +0000
> @@ -0,0 +1,89 @@
> +===================
> +Mailman REST Client
> +===================
> +
> +Domains
> +=======
> +
> + # The test framework starts out with an example domain, so let's delete
> + # that first.
> + >>> from mailman.interfaces.domain import IDomainManager
> + >>> from zope.component import getUtility
> + >>> domain_manager = getUtility(IDomainManager)
> +
> + >>> domain_manager.remove('example.com')
> + <Domain example.com...>
> + >>> transaction.commit()
The test infrastructure currently sets up this sample domain. I wonder
whether the REST client tests will more often want to start from a clean
slate, or want this example domain. I think perhaps the former, so I should
probably add a layer that is just like the ConfigLayer, but has an empty
testSetUp(). (You don't have to worry about that for now. :)
> +In order to add new lists first a new domain has to be added.
> +
> + >>> from mailmanclient.rest import MailmanRESTClient, MailmanRESTClientError
> + >>> c = MailmanRESTClient('localhost:8001')
> + >>> c.create_domain('example.com')
> + True
What do you think about having proxy objects for Domains, Lists, etc.?
Launchpadlib works like this and it provides more of a natural, object
oriented API to clients, rather than an XMLRPC style seen here.
So for example, .create_domain() would return a Domain surrogate, and you'd
call things like .get_lists() and .create_list() on that object.
How much harder do you think it would be to do something like that, and do you
think it would be worth it?
> +
> +
> +Mailing lists
> +=============
> +
> +You can get a lists of all lists by calling get_lists(). If no lists have been created yet, MailmanRESTClientError is raised.
Please wrap narrative to 78 characters.
> +
> + >>> lists = c.get_lists()
> + Traceback (most recent call last):
> + ...
> + MailmanRESTClientError: No mailing lists found
I think it might be better to return an empty list instead of raising an
exception here. Think about application code like so:
# Display all the current lists.
try:
lists = c.get_lists()
except MailmanRESTClientError:
# Are we sure we got "No mailing lists found" or did some other
# generic client error occur?
lists = []
for mailing_list in lists:
# ...
The thing is, having no mailing lists isn't an error condition, so this should
probably return an empty (Python) list rather than raise an exception.
> +
> +Lists can be created by calling create_list('listname at domain.tld').
> +
> + >>> c.create_list('test-one at example.com')
> + True
> + >>> c.create_list('test-two at example.com')
> + True
> +
> +If there are any mailing lists a list of dicts is returned.
> +
> + >>> lists = c.get_lists()
> + >>> lists
> + [{u'real_name': u'Test-one', u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com', u'http_etag': u'"5e99519ef1b823a52254b77e89bec54fbd17bef0"', u'host_name': u'example.com', u'fqdn_listname': u'test-one at example.com', u'list_name': u'test-one'}, {u'real_name': u'Test-two', u'self_link': u'http://localhost:8001/3.0/lists/test-two@example.com', u'http_etag': u'"a05542c9faa07cbe2b8fdf8a1655a2361ab365f2"', u'host_name': u'example.com', u'fqdn_listname': u'test-two at example.com', u'list_name': u'test-two'}]
Unfortunately, this is too fragile because it depends on dictionary sort
order, which isn't defined in Python, and has definitely changed between
Python versions!
This is why I added the dump_json() helper in
src/mailman/tests/test_documentation.py. That helper isn't entirely
appropriate for your tests because you don't explicitly pass in a url; that's
basically embedded in .get_lists and the client. But there's enough
commonality that dump_json() should be refactored into something that can be
shared.
Do you want to take a crack at that?
> +
> +Information about a specific list can be read (returns a single dict).
> +
> + >>> list = c.read_list('test-one at example.com')
> + >>> list
> + {u'real_name': u'Test-one', u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com', u'http_etag': u'"5e99519ef1b823a52254b77e89bec54fbd17bef0"', u'host_name': u'example.com', u'fqdn_listname': u'test-one at example.com', u'list_name': u'test-one'}
I'm not sure 'reading' a list is the right phrase here. "Reading" a list
implies to me reading its archive. Maybe .get_list() here?
> +
> +
> +Membership
> +==========
> +
> +Users can subscribe to a mailing list using the list's name, their email address, as well as their name.
> +
> + >>> c.subscribe_list('test-one at example.com', 'aperson at example.com', 'Garp')
> + True
> + >>> c.subscribe_list('test-one at example.com', 'bperson at example.com', 'Jenny')
> + True
> + >>> c.subscribe_list('test-two at example.com', 'bperson at example.com', 'Jenny')
> + True
Here's a place where a more object-oriented client might work better. For
example, if we had both a List and User object, then depending on which one
you had, you could do a simpler subscription. E.g. if .read_list/.get_list
returned a List object, you'd do something like this:
>>> test_one = c.get_list('test-one at example.com')
>>> member = test_one.subscribe('aperson at example.com', 'Garp')
Conversely, if you had a User object, you could do:
>>> aperson = c.get_user('aperson at example.com')
>>> aperson.subscribe('test-one at example.com')
> +Members of all mailings list can be listed (returns a list of dicts).
> +
> + >>> all_members = c.get_members()
> + >>> all_members
> + [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example.com', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/bperson@example.com', u'http_etag': u'"344ef9f37f59cce14a81b688dd1fa3d39112e6b5"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-two@example.com/member/bperson@example.com', u'http_etag': u'"a5c962186200af483f6508dfbabca7a4a78ef309"'}]
Here's that dict ordering problem again.
> +
> +As well as the members of a specific lists:
> +
> + >>> list_members = c.get_members('test-one at example.com')
> + >>> list_members
> + [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example.com', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/bperson@example.com', u'http_etag': u'"344ef9f37f59cce14a81b688dd1fa3d39112e6b5"'}]
> +
> +Lists can be unsubscribed by using list name and the member's email address
> +
> + >>> c.leave_list('test-one at example.com', 'bperson at example.com')
> + True
> +
> +After leaving the list only the remaining members are shown:
> +
> + >>> new_members = c.get_members('test-one at example.com')
> + >>> new_members
> + [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example.com', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}]
The same API principles apply here too.
=== added directory 'src/mailmanclient'
=== added file 'src/mailmanclient/__init__.py'
=== added file 'src/mailmanclient/rest.py'
--- src/mailmanclient/rest.py 1970-01-01 00:00:00 +0000
+++ src/mailmanclient/rest.py 2010-06-25 16:50:42 +0000
> @@ -0,0 +1,248 @@
> +# Copyright (C) 2009-2010 by the Free Software Foundation, Inc.
> +#
> +# This file is part of GNU Mailman.
> +#
> +# GNU Mailman is free software: you can redistribute it and/or modify it under
> +# the terms of the GNU General Public License as published by the Free
> +# Software Foundation, either version 3 of the License, or (at your option)
> +# any later version.
> +#
> +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU General Public License along with
> +# GNU Mailman. If not, see <http://www.gnu.org/licenses/>.
I hate to be so nitpicky on style, but I'm gonna anyway. :) A few things will
help consistency with other code, so I'll be kind of anal about it for now.
template.py in the top level directory is a good place to start with a new
Python file. It contains the license header, a template module docstring,
and some important __future__ imports. It also forces all unadorned classes
to be new style (through the __metaclass__ assignment), and adds an empty
__all__ for publicly exported symbols.
> +
> +import json
> +from httplib import HTTPConnection, HTTPException
> +from urllib import urlencode
See src/mailman/docs/STYLEGUIDE.txt
-----snip snip-----
import json
from httplib import HTTPConnection, HTTPException
from urllib import urlencode
-----snip snip-----
(two blank lines between top level constructs)
> +
> +class MailmanRESTClientError(Exception):
> + """An exception thrown by the Mailman REST API client."""
> + pass
> +
(blank line needed)
> +class MailmanRESTClient(object):
> + """A thin client wrapper for the Mailman REST API."""
> +
> + def __init__(self, host):
> + self.host = host
> + if self.host[-1] == '/':
> + self.host = self.host[:-1]
> +
> + # general header information
This should be a complete sentence, e.g.:
# Include general header information.
> + self.headers = {
> + "User-Agent": "MailmanRESTClient",
> + "Accept": "text/plain",
> + }
I usually prefer single quotes when possible (one less shift key to hold down
:), and the closing brace should be indented 4 spaces.
> +
> + try:
> + self.c = HTTPConnection(self.host)
> + except:
> + raise MailmanRESTClientError('Could not connect to server')
The bare except is problematic; it's generally not a good idea to use them.
In this case, it's probably better to just let the HTTPConnection exception
flow up uncaught.
> +
> + def __repr__(self):
> + return '<MailmanRESTClient: %s>' % self.host
> +
> + def _get_url(self, path):
> + try:
> + self.c.request('GET', path, None, self.headers)
> + except:
> + raise MailmanRESTClientError('Error sending request')
Similarly here. I don't think we need to turn all exceptions into
MailmanRESTClientErrors. Let's reserve that exception hierarchy for
exceptions specifically in our library.
> +
> + try:
> + r = self.c.getresponse()
> + raw_data = r.read()
> + if len(raw_data) == 0:
> + raise None
You're actually not allowed to raise None (I had to try it myself to see :).
This might be another case of just letting the original exception percolate
up:
>>> json.loads('')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python2.6/json/__init__.py", line 307, in loads
return _default_decoder.decode(s)
File "/usr/lib/python2.6/json/decoder.py", line 319, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/lib/python2.6/json/decoder.py", line 338, in raw_decode
raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded
I'm a little less sure about this one, but that probably calls for just
letting the json.loads() fail until there's an overriding reason to mutate
it.
> + data = json.loads(raw_data)
> + except:
> + raise MailmanRESTClientError('Error sending request')
> + finally:
> + self.c.close()
> +
> + return data
> +
> + def _post_url(self, path, data):
> + self.headers['Content-type'] = "application/x-www-form-urlencoded"
> +
> + try:
> + self.c.request('POST', path, urlencode(data), self.headers)
> + except:
> + raise MailmanRESTClientError('Could not send request')
Again, probably don't need to try/except this.
> +
> + try:
> + r = self.c.getresponse()
> +
> + if r.status == 201:
> + return True
> + else:
> + return r.status
Interesting. Why not always just return the status code? It seems like it
would be better to always return an integer and let the client decide what to
do with the status code, rather than transform just 201 into True.
> + finally:
> + self.c.close()
> +
> + def _delete_url(self, path):
> + try:
> + self.c.request('DELETE', path, None, self.headers)
> + except:
> + raise MailmanRESTClientError('Could not send request')
> + finally:
> + self.c.close()
Similar.
> +
> + def _put_url(self, path):
> + """To be implemented...
> + """
You can use an XXX comment like so:
"""XXX 2010-06-29 florian: to be implemented."""
That slightly suggestive XXX is an old Guido tradition, but there are lots of
good tools that search for them and remind you about them. E.g. pylint.
Including the date and your Launchpad id is a good idea to identify who wrote
the comment and how old it is, without having to reconstruct it from the vcs
history.
> + pass
> +
> + def get_lists(self):
> + """Get a list of mail lists.
> +
> + returns a list of dicts
You can use epydoc rest markup for describing the parameters and return values
for a method.
http://epydoc.sourceforge.net/manual-fields.html#epydoc-fields
There are lots of examples sprinkled about in the current Mailman code.
> + """
> + try:
> + r = self._get_url('/3.0/lists')
> + except:
> + raise MailmanRESTClientError('Could not send request')
> +
> + if 'entries' not in r:
> + raise MailmanRESTClientError('No mailing lists found')
> +
> + return r['entries']
> +
> + def read_list(self, list_name):
> + """Get information about list.
> +
> + :param list_name: name of the list to get information about
> + :type list_name: string
> + :rtype: dict
Yep, like that!
> + """
> + try:
> + r = self._get_url('/3.0/lists/' + list_name)
> + except:
> + raise MailmanRESTClientError('Unable to get info about list')
> +
> + return r
> +
> + def create_list(self, fqdn_listname, **kwargs):
> + """Create a new list.
> +
> + :param fqdn_listname: the name of the list including the @ and the domain name. eg. test at example.com
This needs to be line wrapped to 78 characters. You can indent continuation
lines.
> + :type fqdn_listname: string
> + :rtype: None
> + """
> + data = {
> + 'fqdn_listname': fqdn_listname
Dedent 4 spaces and end in a comma.
> + }
Indent 4 spaces.
> + data.update(**kwargs)
> + try:
> + return self._post_url('/3.0/lists', data)
> + except MailmanRESTClientError, e:
> + raise MailmanRESTClientError(e)
Similar, no need to catch this exception.
> +
> +
> + def subscribe_list(self, fqdn_listname, address, real_name, **kwargs):
> + """Add an address to a list.
> +
> + :param fqdn_listname: the name of the list .
> + :type fqdn_listname: string
> + :param address: email address to add to the list.
> + :type address: string
> + :param real_name: the "real" name for the address to be addded
> + :type real_name: string
> + """
> +
> + data = {
> + 'fqdn_listname': fqdn_listname,
> + 'address': address,
> + 'real_name': real_name
Dedent all lines 4 spaces and end the last one in a comma.
> + }
Indent 4 spaces.
> + data.update(**kwargs)
> + try:
> + r = self._post_url('/3.0/members', data)
> + except:
> + raise MailmanRESTClientError('unable to join list')
Don't catch.
> +
> + return r
> +
> + def leave_list(self, fqdn_listname, address):
> + """Remove an address from a list.
> +
> + :param fqdn_listname: the name of the list.
> + :type fqdn_listname: string
> + :param address: email address that is leaving the list
> + :type address: string
> + :rtype: None
> + """
> + try:
> + r = self._delete_url('/3.0/lists/' + fqdn_listname + '/member/' + address )
Line length. You can probably just write this like:
return self._delete_url(
'/3.0/lists/' + fqdn_listname + '/member/' + address)
(Note, no space before the close paren.)
> + except:
> + raise MailmanRESTClientError('unable to leave list')
> +
> + return True
> +
> + def get_members(self, fqdn_listname = None):
No spaces around the = sign.
> + """Get a list of all members for all lists.
> +
> + :rtype: list of dicts
> + :type fqdn_listname: string
> + """
> +
> + if fqdn_listname != None:
> + url = '/3.0/lists/' + fqdn_listname + '/roster/members'
> + else:
> + url = '/3.0/members'
> + try:
> + r = self._get_url(url)
> + except:
> + raise MailmanRESTClientError('Could not complete request')
> +
> + if 'entries' not in r:
> + raise MailmanRESTClientError('Could not find any members')
Maybe just return the empty list here?
> +
> + return r['entries']
> +
> + def get_domains(self):
> + """Get a list of domains.
> +
> + :rtype: list of dicts
> + """
> + try:
> + r = self._get_url('/3.0/domains')
> + except:
> + raise MailmanRESTClientError('Could not complete request')
> + if 'entries' not in r:
> + raise MailmanRESTClientError('Could not find any domains')
And here, an empty list would be fine.
> +
> + return r['entries']
> +
> + def read_domain(self, domain_name):
> + """Get information about a specific domain.
> +
> + :param domain_name: the name of the domain.
> + :rtype: dict
> + """
> + try:
> + r = self._get_url('/3.0/domains/' + domain_name)
> + except:
> + raise MailmanRESTClientError('Unable to read domain')
> +
> + return r
> +
> + def create_domain(self, email_host, **kwargs):
> + """Create a new domain name.
> +
> + :param email_host: domain name to create
> + :type email_host: string
> + """
> + data = {
> + 'email_host': email_host,
> + }
> + data.update(**kwargs)
> + try:
> + r = self._post_url('/3.0/domains', data)
> + except:
> + raise MailmanRESTClientError('Unable to create domain')
> +
> + return r
> +
Similar comments here.
This is a great first draft. Please let me know if any of my comments don't
make sense or you have questions about them. Thanks!
-Barry
--
https://code.launchpad.net/~flo-fuchs/mailman/restclient/+merge/28522
Your team Mailman Coders is requested to review the proposed merge of lp:~flo-fuchs/mailman/restclient into lp:mailman.
More information about the Mailman-coders
mailing list