Review: Resubmit I did some fixes and improvements like suggested in the last review...
+ entry 0: + ... + self_link: http://localhost:8001/3.0/lists/test- one@example.com/member/jack@example.com + entry 1: + ... + self_link: http://localhost:8001/3.0/lists/test- one@example.com/member/meg@example.com + entry 2: + ... + self_link: http://localhost:8001/3.0/lists/test- two@example.com/member/jack@example.com
The client is returning json here, right?
Nope, the client never returns json. Either HTTP status codes or lists/dicts are returned.
Should we be using httplib2 and urllib2 here? See the implementation of dump_json().
Done.
+ def _delete_request(self, path): + """Send an HTTP DELETE request. + + :param path: the URL to send the DELETE request to + :type path: string + :return: request status code + :rtype: string + """ + try: + self.c.request('DELETE', path, None, self.headers) + r = self.c.getresponse() + return r.status + finally: + self.c.close()
I wonder if this duplication can be refactored?
There's only one http request method now.
+ def _validate_email_host(self, email_host): + """Validates a domain name. + + :param email_host: the domain str to validate + :type email_host: string + """ + pat = re.compile('^[-a-z0-9\.]+\.[-a-z]{2,4}$', re.IGNORECASE) + if not pat.match(email_host): + raise MailmanRESTClientError('%s is not a valid domain name' % email_host)
Won't the Mailman core refuse to create a domain if it's not valid? It might still be worth doing client-side validation, but I would expect that more in some webui JavaScript. What's the advantage of doing this extra check (which might be different than what happens in the core)?
I didn't know if the core does email validation. Also, the django app does some validation. So I removed it.
I wonder if this method is necessary. In general, attributes are preferred over accessors, and you've already got a public one right here! So clients can do:
>>> my_domain = client.get_domain('example.com') >>> my_domain.domain_info ...
directly. In fact, for polymorphism, maybe the attribute should just be called 'info'?
Done. -- 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.