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@domain.tld'). + + >>> c.create_list('test-one@example.com') + True + >>> c.create_list('test-two@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@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@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@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@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@example.com', 'aperson@example.com', 'Garp') + True + >>> c.subscribe_list('test-one@example.com', 'bperson@example.com', 'Jenny') + True + >>> c.subscribe_list('test-two@example.com', 'bperson@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@example.com') >>> member = test_one.subscribe('aperson@example.com', 'Garp') Conversely, if you had a User object, you could do: >>> aperson = c.get_user('aperson@example.com') >>> aperson.subscribe('test-one@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....', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/bperson@example....', u'http_etag': u'"344ef9f37f59cce14a81b688dd1fa3d39112e6b5"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-two@example.com/member/bperson@example....', 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@example.com') + >>> list_members + [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example....', u'http_etag': u'"925a22dc585a1c713d79912bd951f4cbb358dae2"'}, {u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/bperson@example....', u'http_etag': u'"344ef9f37f59cce14a81b688dd1fa3d39112e6b5"'}] + +Lists can be unsubscribed by using list name and the member's email address + + >>> c.leave_list('test-one@example.com', 'bperson@example.com') + True + +After leaving the list only the remaining members are shown: + + >>> new_members = c.get_members('test-one@example.com') + >>> new_members + [{u'self_link': u'http://localhost:8001/3.0/lists/test-one@example.com/member/aperson@example....', 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@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.