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.
_______________________________________________
Mailman-coders mailing list
[email protected]
http://mail.python.org/mailman/listinfo/mailman-coders
