+1

On 1/28/14, 18:41, Andrey Petrov wrote:
>
> In urllib3/util/retry.py:
>
> > +        :rtype: bool
> > +        """
> > +        return (resp is not None
> > +                and meth in self.method_whitelist
> > +                and resp.status in self.codes_whitelist)
> > +
> > +    def _is_read_error(self, err):
> > +        """ Check if the server had an error responding to the request
> > +
> > +        Even though we didn't get a response back from the server, these
> > +        exceptions are different than connection errors, because they imply
> > +        the the remote server accepted the request. The server may have begun
> > +        processing the request and performed some side effects (wrote data to a
> > +        database, sent a message, etc).
> > +        """
> > +        if isinstance(err, ReadTimeoutError):
>
> Could just do |return isinstance(err, ReadTimeoutError) or 
> isinstance(err, HTTPLIB_READ_EXCEPTIONS)|
>
> Or better yet, could rename it to |READ_EXCEPTIONS and 
> add|ReadTimeoutError|to the list, then it's just|return 
> isinstance(err, READ_EXCEPTIONS)`.
>
> At that point, I'd get rid of |_is_read_error(...)| altogether which 
> would make the |increment(..)| code more linear to read. I think this 
> is a good idea in general, to get rid of these one-liner helpers 
> unless they add some other value (ie. reused in other places or useful 
> for overriding).
>
> Also I'd add |READ_EXCEPTIONS| as a static member of the |Retry| class 
> rather than module-global, so it could be overridden per-instance.
>
> —
> Reply to this email directly or view it on GitHub 
> <https://github.com/shazow/urllib3/pull/326/files#r9254925>.
>