[Merge] lp:~flo-fuchs/mailman/restclient into lp:mailman

Florian Fuchs flo.fuchs at gmail.com
Wed Jul 7 09:45:49 CEST 2010


First of all: Thanks a lot for that extensive review - I really, really appreciate getting this kind of detailed comment!


Exceptions
==========
MailmanRESTClientError is only used for email- and domain-validation. Apart from that the original Exceptions don't get catched any more.

If there are no mailing lists yet, an empty list is returned instead of an Exception. 


Proxy Objects
=============
> 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.

Great idea! I've added two sub-classes two return as List and Domain objects like:
domain = client.get_domain('example.com') 
list = domain.create_list('test')
and so on...
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?

Generally I'd say "get_singularterm" and "create_singularterm" (get_list(), create_domain() etc.) should return an object with helpful methods and "get_pluralterm"/"create_pluralterm" (get_lists, get_members) should return lists oder dicts...


Style issues
============
I've fixed all the issues according to the style guide and pep8. At least I hope so... Lesson learned... ;-) 


Func name issue
===============

> 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?

I was thinking in CRUD terms where I understand "reading" more like getting a db record. But I agree, it's a little strange here. So I changed it to get_list().


List order
==========
> 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.

I'm not sure if a func name like dump_json is a little confusing in that context since the client doesn't return any json. Maybe we could rename the function into dump_rest_output() (or similar) and make url, method optional? So if url is set, the rest server is called; if not, only data is sorted. `data` would then either server as a parm for POST/PUT content or as a dict to sort... 

For now I've added the sort logic to the test (only a couple of lines). 



So, thanks again for reviewing. On to the next round...? :-)

Florian

-- 
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