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.

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.