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.