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

I did some fixes and improvements like suggested in the last review...

> > + entry 0:
> > + ...
> > + self_link: http://localhost:8001/3.0/lists/test-
> <email address hidden><email address hidden>
> > + entry 1:
> > + ...
> > + self_link: http://localhost:8001/3.0/lists/test-
> <email address hidden><email address hidden>
> > + entry 2:
> > + ...
> > + self_link: http://localhost:8001/3.0/lists/test-
> <email address hidden><email address hidden>
>
> 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.