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- > [email protected]/member/[email protected] > > + entry 1: > > + ... > > + self_link: http://localhost:8001/3.0/lists/test- > [email protected]/member/[email protected] > > + entry 2: > > + ... > > + self_link: http://localhost:8001/3.0/lists/test- > [email protected]/member/[email protected] > > 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. _______________________________________________ Mailman-coders mailing list [email protected] http://mail.python.org/mailman/listinfo/mailman-coders
