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...? :-)
First of all: Thanks a lot for that extensive review - I really, really appreciate getting this kind of detailed comment!
Exceptions ntError is only used for email- and domain-validation. Apart from that the original Exceptions don't get catched any more.
==========
MailmanRESTClie
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: get_domain( 'example. com') create_ list('test' )
domain = client.
list = domain.
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 tests/test_ documentation. py. That helper isn't entirely
==========
> This is why I added the dump_json() helper in
> src/mailman/
> 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