Code review comment for lp:~flo-fuchs/mailman/restclient

Revision history for this message
Florian Fuchs (flo-fuchs) wrote :

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

« Back to merge proposal