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@example.com + ... + +Later the new list can be instanciated using get_list():
s/instanciated/instantiated/
+ + >>> some_list = client.get_list('test-one@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@example.com + ... + entry 1: + fqdn_listname: test-two@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@example.com', 'Jack') + 201 + >>> new_list.subscribe('meg@example.com', 'Meg') + 201 + >>> another_list.subscribe('jack@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@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.