[Merge] lp:~flo-fuchs/mailman/restclient into lp:mailman
Barry Warsaw
barry at canonical.com
Fri Jul 16 16:43:39 CEST 2010
Review: Needs Fixing
Thanks for all the great updates. Things look pretty good, and I have only a
few minor issues to comment on. I think we're almost ready to merge it!
Great work.
On the dump_json() issue, i just thought of something: since you're only
displaying dictionaries, perhaps pprint will do the trick. In Python 2.6,
pprint.pprint() sorts the dictionary elements I believe.
>>> import os
>>> from pprint import pprint
>>> pprint(dict(os.environ))
{'COLUMNS': '79',
...
'DISPLAY': ':0.0',
'EMACS': 't',
...
You asked:
>No user object yet: What do you think: Are we talking about a User with n
>email addresses and n memberships here or more like User = Membership?
I think we should stick fairly close to the internal model here. So 'Users'
represent people, 'Addresses' represent their email addresses, which are
usually associated with a user, and 'Member' joins an address to a mailing
list with a given role. Only indirectly can you get at the user for a member
(i.e. through its address).
I agree about singular/plural terms.
-B
=== 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-07-15 14:02:55 +0000
> @@ -0,0 +1,129 @@
> +===================
> +Mailman REST Client
> +===================
> +
> + # 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()
> +
> +First let's get an instance of MailmanRESTClient.
> +
> + >>> from mailmanclient.rest import MailmanRESTClient, MailmanRESTClientError
> + >>> client = MailmanRESTClient('localhost:8001')
> +
> +So far there are no lists.
> +
> + >>> lists = client.get_lists()
> + >>> print lists
> + []
I think you can just do
>>> client.get_lists()
[]
> +
> +
> +Domains
> +=======
> +
> +In order to add new lists first a new domain has to be added.
> +
> + >>> new_domain = client.create_domain('example.com')
> + >>> new_domaininfo = new_domain.get_domaininfo()
> + >>> for key in sorted(new_domaininfo):
> + ... print '{0}: {1}'.format(key, new_domaininfo[key])
> + base_url: http://example.com
> + ...
> +
> +Later the domain object can be instanciated using get_domain()
> +
> + >>> my_domain = client.get_domain('example.com')
> +
> +
> +Mailing lists
> +=============
> +
> +Now let's add some mailing lists.
> +
> + >>> new_list = my_domain.create_list('test-one')
> +
> +Lets add another list and get some information on the list.
s/Lets/let's/
> +
> + >>> another_list = my_domain.create_list('test-two')
> + >>> another_listinfo = another_list.get_listinfo()
> + >>> for key in sorted(another_listinfo):
> + ... print '{0}: {1}'.format(key, another_listinfo[key])
> + fqdn_listname: test-two at example.com
> + ...
> +
> +Later the new list can be instanciated using get_list():
s/instanciated/instantiated/
> +
> + >>> some_list = client.get_list('test-one at example.com')
> +
> +The lists have been added and get_lists() returns a list of dicts, sorted
> +by fqdn_listname.
> +
> + >>> lists = client.get_lists()
> + >>> for i, list in enumerate(lists):
> + ... print 'entry %d:' % i
> + ... for key in sorted(list):
> + ... print ' {0}: {1}'.format(key, list[key])
> + entry 0:
> + fqdn_listname: test-one at example.com
> + ...
> + entry 1:
> + fqdn_listname: test-two at example.com
> + ...
> +
> +
> +Membership
> +==========
> +
> +Since we now have a list we should add some members to it (.subscribe()
> +returns an HTTP status code, ideally 201)
> +
> + >>> new_list.subscribe('jack at example.com', 'Jack')
> + 201
> + >>> new_list.subscribe('meg at example.com', 'Meg')
> + 201
> + >>> another_list.subscribe('jack at example.com', 'Jack')
> + 201
> +
> +We can get a list of all members:
> +
> + >>> members = client.get_members()
> + >>> for i, member in enumerate(members):
> + ... print 'entry %d:' % i
> + ... for key in sorted(member):
> + ... print ' {0}: {1}'.format(key, member[key])
The other thing to think about is, if you repeat similar code in the doctest,
you can always refactor them into a function defined in the doctest.
> + entry 0:
> + ...
> + self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/jack@example.com
> + entry 1:
> + ...
> + self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/meg@example.com
> + entry 2:
> + ...
> + self_link: http://localhost:8001/3.0/lists/test-two@example.com/member/jack@example.com
The client is returning json here, right?
> +
> +Or just the members of a specific list:
> +
> + >>> new_list_members = new_list.get_members()
> + >>> for i, member in enumerate(new_list_members):
> + ... print 'entry %d:' % i
> + ... for key in sorted(member):
> + ... print ' {0}: {1}'.format(key, member[key])
> + entry 0:
> + http_etag: ...
> + self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/jack@example.com
> + entry 1:
> + http_etag: ...
> + self_link: http://localhost:8001/3.0/lists/test-one@example.com/member/meg@example.com
> +
> +After a while Meg decides to unsubscribe from the mailing list (like
> +.subscribe() .unsubscribe() returns an HTTP status code, ideally 200).
> +
> + >>> new_list.unsubscribe('meg at example.com')
> + 200
> +
=== 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-07-15 14:02:55 +0000
> @@ -0,0 +1,321 @@
> +# Copyright (C) 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/>.
> +
> +"""A client library for the Mailman REST API."""
> +
> +
> +from __future__ import absolute_import, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> + 'MailmanRESTClient',
> + 'MailmanRESTClientError',
> + ]
> +
> +
> +import json
> +import re
Believe it or not, these lines should be reversed. I know it's weird but non
from-import-* imports get sorted in increasing length. Just a little quirk I
inherited from Guido. :)
import re
import json
> +
> +from httplib import HTTPConnection, HTTPException
> +from urllib import urlencode
Should we be using httplib2 and urllib2 here? See the implementation of
dump_json().
> +
> +
> +class MailmanRESTClientError(Exception):
> + """An exception thrown by the Mailman REST API client."""
> +
You don't need that extra blank line. Strictly speaking, you don't need the
following 'pass' line either!
> + pass
> +
> +
> +class MailmanRESTClient(object):
The base class isn't necessary here, because the '__metaclass__ = type' at the
top of the module already makes all unadorned classes default to new style.
> + """A wrapper for the Mailman REST API."""
> +
> + def __init__(self, host):
> + """Connect to host
First line should end in a period, and there should be a blank line
following. Also, I try to make :param: descriptions full sentences too
(capitalized and ending in a period). Not the :type: description though.
Apply these rules to all the following docstrings.
> + :param host: the host name of the REST API
> + :type host: string
> + """
> + self.host = host
> + # If there is a trailing slash remove it
> + if self.host[-1] == '/':
> + self.host = self.host[:-1]
> + # Include general header information
> + self.headers = {
> + 'User-Agent': 'MailmanRESTClient',
> + 'Accept': 'text/plain',
> + }
> + self.c = HTTPConnection(self.host)
Abbreviations == bad! :) Well, usually. ;)
You can use self.connection or self.conn here to make it a little more
readable. OTOH, in dump_json(), we create a new HTTP() instance for every
request. I wonder if that wouldn't be a better way to go. IOW, what's the
advantage of caching this HTTPConnection instance in this attribute?
> +
> + def __repr__(self):
> + return '<MailmanRESTClient: %s>' % self.host
> +
> + def _get_request(self, path):
> + """Send an HTTP GET request.
> +
> + :param path: the URL to send the GET request to
> + :type path: string
> + :rtype: list
Add an :return: to describe what gets returned.
> + """
> + try:
> + self.c.request('GET', path, None, self.headers)
> + r = self.c.getresponse()
> + raw_data = r.read()
> + data = json.loads(raw_data)
> + return data
> + finally:
> + self.c.close()
> +
> + def _post_request(self, path, data):
> + """Send an HTTP POST request.
> +
> + :param path: the URL to send POST request to
> + :type path: string
> + :param data: the post data to send
> + :type data: dict
> + :return: request status code
> + :rtype: string
> + """
> + self.headers['Content-type'] = "application/x-www-form-urlencoded"
> + try:
> + self.c.request('POST', path, urlencode(data), self.headers)
> + r = self.c.getresponse()
> + return r.status
> + finally:
> + self.c.close()
> +
> + def _delete_request(self, path):
> + """Send an HTTP DELETE request.
> +
> + :param path: the URL to send the DELETE request to
> + :type path: string
> + :return: request status code
> + :rtype: string
> + """
> + try:
> + self.c.request('DELETE', path, None, self.headers)
> + r = self.c.getresponse()
> + return r.status
> + finally:
> + self.c.close()
I wonder if this duplication can be refactored?
> +
> + def _put_request(self, path, data):
> + """Send an HTTP PUT request.
> +
> + :param path: the URL to send the PUT request to
> + :type path: string
> + :param data: the put data to send
> + :type data: dict
> + :return: request status code
> + :rtype: string
> + """
> + self.headers['Content-type'] = "application/x-www-form-urlencoded"
> + try:
> + self.c.request('PUT', path, urlencode(data), self.headers)
> + r = self.c.getresponse()
> + return r.status
> + finally:
> + self.c.close()
> +
> + def _validate_email_host(self, email_host):
> + """Validates a domain name.
> +
> + :param email_host: the domain str to validate
> + :type email_host: string
> + """
> + pat = re.compile('^[-a-z0-9\.]+\.[-a-z]{2,4}$', re.IGNORECASE)
> + if not pat.match(email_host):
> + raise MailmanRESTClientError('%s is not a valid domain name' % email_host)
Won't the Mailman core refuse to create a domain if it's not valid? It might
still be worth doing client-side validation, but I would expect that more in
some webui JavaScript. What's the advantage of doing this extra check (which
might be different than what happens in the core)?
> +
> + def _validate_email_address(self, email_address):
> + """Validates an email address.
> +
> + :param email_address: the email string to validate
> + :type email_address: string
> + """
> + pat = re.compile('^[-a-z0-9_.]+@[-a-z0-9\.]+\.[a-z]{2,6}$',re.IGNORECASE)
> + if not pat.match(email_address):
> + raise MailmanRESTClientError('%s is not a valid email_address' % email_address)
Same question here.
> +
> + def create_domain(self, email_host):
> + """Create a domain and return a domain object.
> +
> + :param email_host: host domain to create
> + :type email_host: string
> + :rtype object
> + """
> + self._validate_email_host(email_host)
> + data = {
> + 'email_host': email_host,
> + }
> + r = self._post_request('/3.0/domains', data)
> + return _Domain(self.host, email_host)
Better to use 'response' rather than 'r'.
> +
> + def get_domain(self, email_host):
> + """Return a domain object.
> +
> + :param email_host: host domain
> + :type email_host: string
> + :rtype object
> + """
> + self._validate_email_host(email_host)
> + return _Domain(self.host, email_host)
> +
> + def get_lists(self):
> + """Get a list of all mailing list.
> +
> + :return: a list of dicts with all mailing lists
> + :rtype: list
> + """
> + r = self._get_request('/3.0/lists')
> + if 'entries' not in r:
> + return []
> + else:
> + # Return a dict with entries sorted by fqdn_listname
> + return sorted(r['entries'],
> + key=lambda list: list['fqdn_listname'])
lambdas are kind of icky, how about:
# Move this to the imports at the top of the file.
from operator import itemgetter
return sorted(response['entries'], key=itemgetter('fqdn_listname'))
> +
> + def get_list(self, fqdn_listname):
> + """Find and return a list object.
> +
> + :param fqdn_listname: the mailing list address
> + :type fqdn_listname: string
> + :rtype: object
> + """
> + self._validate_email_address(fqdn_listname)
> + return _List(self.host, fqdn_listname)
> +
> + def get_members(self):
> + """Get a list of all list members.
> +
> + :return: a list of dicts with the members of all lists
> + :rtype: list
> + """
> + r = self._get_request('/3.0/members')
> + if 'entries' not in r:
> + return []
> + else:
> + return sorted(r['entries'],
> + key=lambda member: member['self_link'])
See above.
> +
> +
> +class _Domain(MailmanRESTClient):
> + """A domain wrapper for the MailmanRESTClient."""
> +
> + def __init__(self, host, email_host):
> + """Connect to host and get list information.
> +
> + :param host: the host name of the REST API
> + :type host: string
> + :param email_host: host domain
> + :type email_host: string
> + :rtype: object
> + """
> + MailmanRESTClient.__init__(self, host)
For new-style classes:
super(_Domain, self).__init__(host)
> + self.domain_info = self._get_request('/3.0/domains/' + email_host)
> +
> + def get_domaininfo(self):
> + """Get information about the domain.
> +
> + :rtype: dict
> + """
> + return self.domain_info
I wonder if this method is necessary. In general, attributes are preferred
over accessors, and you've already got a public one right here! So clients
can do:
>>> my_domain = client.get_domain('example.com')
>>> my_domain.domain_info
...
directly. In fact, for polymorphism, maybe the attribute should just be
called 'info'?
> +
> + def create_list(self, list_name):
> + """Create a mailing list and return a list object.
> +
> + :param list_name: the name of the list to be created
> + :type list_name: string
> + :rtype: object
> + """
> + fqdn_listname = list_name + '@' + self.domain_info['email_host']
> + self._validate_email_address(fqdn_listname)
> + data = {
> + 'fqdn_listname': fqdn_listname
> + }
> + r = self._post_request('/3.0/lists', data)
> + return _List(self.host, fqdn_listname)
> +
> + def delete_list(self, list_name):
> + return self._delete_request('/3.0/lists/' + fqdn_listname)
> +
> +
> +class _List(MailmanRESTClient):
> + """A mailing list wrapper for the MailmanRESTClient."""
> +
> + def __init__(self, host, fqdn_listname):
> + """Connect to host and get list information.
> +
> + :param host: the host name of the REST API
> + :type host: string
> + :param fqdn_listname: the mailing list address
> + :type fqdn_listname: string
> + :rtype: object
> + """
> + MailmanRESTClient.__init__(self, host)
> + self.list_info = self._get_request('/3.0/lists/' + fqdn_listname)
> +
> + def get_listinfo(self):
> + """Get information about the list.
> +
> + :rtype: dict
> + """
> + return self.list_info
> +
> + def subscribe(self, address, real_name=None):
> + """Add an address to a list.
> +
> + :param address: email address to add to the list.
> + :type address: string
> + :param real_name: the real name of the new member
> + :type real_name: string
> + """
> + self._validate_email_address(address)
> + data = {
> + 'fqdn_listname': self.list_info['fqdn_listname'],
> + 'address': address,
> + 'real_name': real_name
> + }
> + return self._post_request('/3.0/members', data)
> +
> + def unsubscribe(self, address):
> + """Unsubscribe an address to a list.
> +
> + :param address: email address to add to the list.
> + :type address: string
> + :param real_name: the real name of the new member
> + :type real_name: string
> + """
> + self._validate_email_address(address)
> + return self._delete_request('/3.0/lists/' +
> + self.list_info['fqdn_listname'] +
> + '/member/' +
> + address)
> +
> + def get_members(self):
> + """Get a list of all list members.
> +
> + :return: a list of dicts with all members
> + :rtype: list
> + """
> + r = self._get_request('/3.0/lists/' +
> + self.list_info['fqdn_listname'] +
> + '/roster/members')
> + if 'entries' not in r:
> + return []
> + else:
> + return sorted(r['entries'],
> + key=lambda member: member['self_link'])
> +
--
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