Replacing usage of isinstance in requests

https://github.com/sigmavirus24/requests/commit/4dd3d1a1a2534f2996a368ebe26114bf974e15f9#commitcomment-1904600

For reference, I'm going to copy the comments into here.

---

@piotr-dobrogost

Explicit type checking is code smell. We should be checking for iterability not types. See http://stackoverflow.com/questions/1952464/

---

@sigmavirus24

I was following the style of the rest of requests, if wanted I'll clean up this and the other places I saw it.

---

@piotr-dobrogost

@kennethreitz What do you think?

---

@kennethreitz 

iterability is always preferred, but not always possible in requests because of the way certain types work (strings, dicts)

---

@sigmavirus24 

@kennethreitz correct me if I'm wrong, but I believe you're against this. And if I remember correctly I had submitted an earlier PR that tried to do something similar to what @piotr-dobrogost is suggesting in `_encode_data` which failed when passed a dictionary. For reference in python 2 and 3, the following works

```
>>> a = [1, 2, 3]
>>> b = {4: 5}
>>> a.extend(b)
# a == [1, 2, 3, 4]
```

Which may not be entirely obvious to some and certainly might seem like unintended behavior. Likewise,

```
>>> a = [1, 2, 3]
>>> a.extend('abcdef')
# a == [1, 2, 3, 'a', 'b', 'c', 'd', 'e', 'f']
```

We could always do the complement though (which is far less obvious):

```
if hasattr(hook, '__iter__') and not isinstance(hook, (str, dict)):
# ...
```

---

@piotr-dobrogost 

All we should care here is that we want iterable. It's up to the user to pass iterable he thinks is appropriate. If someone wants to pass "random" iterable then it's not our business to know better and reject it. Yes, there is more type checking in the Requests and I think it should be cleaned as well.

---

@piotr-dobrogost 

@shazow What's your opinion?

---

@shazow 

I would vote for making an is_foo helper somewhere nearby which contains the "is iter and not str/dict" logic or whatever we want it to be. Then re-use that where appropriate. Also name foo something descriptive. is_plural?

As for the logic, I'd be +0 on {iter and not str/dict}. I imagine people may want to pass in arbitrary generators and whatnot.

---

@sigmavirus24 

I think @piotr-dobrogost would just rather the logic `hasattr(foo, '__iter__')` rather than excluding `str`s and `dict`s.

---

@shazow 

In the context of hooks, shouldn't the check just be if it's callable? If not, then extend?

(My previous suggestion was more towards the general style of Requests, rather than this hook-specific example.)

---

@sigmavirus24 

Right, I was following the general style of requests when writing this. But yeah, you're right, if it's callable then it should be append otherwise we should try to extend it.

---

@shazow 

Further: Shouldn't the logic for register_hook and deregister_hook live in the same place as dispatch_hook? If that was the case, then the {check for `__call__` or extend} decision would be very evident. :)

---

@piotr-dobrogost 

Just in case I asked this http://stackoverflow.com/questions/12590494/.

> In the context of hooks, shouldn't the check just be if it's callable? If not, then extend?

Either we check all for callability or none. Why should the first one be better than the rest? :)

---

@sigmavirus24 

So you just want something like:

``` python

if callable(hook):
    self.hooks[event].append(hook)
else:
    self.hooks[event].extend(hook)
```

?

How about a compromise on:

``` python

if callable(hook):
    self.hooks[event].append(hook)
elif hasattr(hook, '__iter__'):
    self.hooks[event].extend(hook)
```

Because calling it with None would cause some issues. If you would rather check that each element of the iterable is callable then simply:

``` python

if callable(hook):
    self.hooks[event].append(hook)
elif hasattr(hook, '__iter__'):
    for h in hook:
        self.register_hook(event, h)
```

That will take care of checking for callability and allow for lists, tuples, sets, etc to be passed.

---

@sigmavirus24 

@piotr-dobrogost, @shazow, @kennethreitz, does anyone mind if we take this to an issue instead of continuing to discuss everything here? An issue would make this more visible to future contributors/users/etc. of requests than it is here.

---

@piotr-dobrogost 

+1

> That will take care of checking for callability and allow for lists, tuples, sets, etc to be passed.

Not only this, this will allow for nested iterables (trees) to be passed as well! :)

---

Well that was painful, I'm all up for fixing this in particular and going through the rest of requests to see if we can get rid of some of these explicit checks unless anyone disagrees.


> Well that was painful

@sigmavirus24 What do you mean? Btw, thanks for creating this issue.


It was painful copying over the comments is all I meant. And no problem. Thanks for bringing up the entirely valid point of the issue. You also likely caught a bug in my logic for registering hooks that I overlooked because I felt bad for delaying the progress of that issue.


So this is what I get looking for usage of `isinstance` in requests.

```
$ grep -n 'isinstance(' requests/*.py
requests/auth.py:167:                if isinstance(x, str):
requests/auth.py:173:                if isinstance(x, str):
requests/cookies.py:169:        if isinstance(value, Morsel):
requests/models.py:135:        if isinstance(cookies, cookielib.CookieJar):
requests/models.py:221:            if isinstance(self.full_url, bytes):
requests/models.py:329:        if isinstance(data, (str, bytes)):
requests/models.py:336:                for v in isinstance(vs, list) and vs or [vs]:
requests/models.py:339:                            (k.encode('utf-8') if isinstance(k, str) else k,
requests/models.py:340:                             v.encode('utf-8') if isinstance(v, str) else v))
requests/models.py:353:        if (not files) or isinstance(self.data, str):
requests/models.py:361:            if isinstance(val, list):
requests/models.py:369:            if isinstance(v, (tuple, list)):
requests/models.py:374:            if isinstance(fp, str):
requests/models.py:376:            if isinstance(fp, bytes):
requests/models.py:411:            if isinstance(scheme, str):
requests/models.py:413:            if isinstance(netloc, str):
requests/models.py:415:            if isinstance(path, str):
requests/models.py:417:            if isinstance(params, str):
requests/models.py:419:            if isinstance(query, str):
requests/models.py:421:            if isinstance(fragment, str):
requests/models.py:465:        if isinstance(hook, (list, tuple, set)):
requests/models.py:513:            if isinstance(self.auth, tuple) and len(self.auth) == 2:
requests/models.py:534:                if isinstance(self.data, str) or isinstance(self.data, builtin_str) or hasattr(self.data, 'read'):
requests/models.py:637:                if isinstance(e, _SSLError):
requests/models.py:639:                elif isinstance(e, TimeoutError):
requests/sessions.py:31:    if isinstance(local_kwarg, str):
requests/sessions.py:93:        if isinstance(cookies, cookielib.CookieJar):
requests/sessions.py:201:        if not isinstance(cookies, cookielib.CookieJar):
requests/utils.py:134:    if isinstance(value, (str, bytes, bool, int)):
requests/utils.py:156:    if isinstance(value, (str, bytes, bool, int)):
requests/utils.py:159:    if isinstance(value, dict):
requests/utils.py:269:    if isinstance(headers, dict):
requests/utils.py:271:    elif isinstance(headers, basestring):
requests/utils.py:273:    elif isinstance(headers, str):
```

**Edit** that's 34 lines it's being used in, mostly in models and utils. The latter is definitely in part my fault.


Line 363 in `models.py` is certainly effective, but could probably use some serious modification. I'm open to suggestions.

**Edit** Also, the tests are failing.


Not all uses of isinstance are bad. :)


Agreed @shazow, I'm probably going to change some of them back and squash the commits before turning that branch into a Pull Request which is probably a while off.


This seems a bit pedantic to me.

Simultaneous support of both Python 2 and 3 will make this difficult, and I'd be _shocked_ if there weren't serious hidden regressions from these changes. What's the tangible benefit? I doubt there is one. Most of this stuff is purely internal anyway. 


@shazow Then please show us which ones are good and why? :) Btw, yours _is_foo helper_ idea was heading in right direction. This is what ABCs in collections module are for although it looks like they specify too few interfaces.


It's not _internal_ because many of these are part of api. I don't see a reason to restrict users to a few built-in collections. Are there any?


Please give me an example of an end-user benefiting change.

(I'm not disagreeing with you, I'd love to see an example)


@piotr-dobrogost For example when the type needs to be checked for py2 vs py3 compatibility.


@kennethreitz
People use different containers - see for example http://sebsauvage.net/python/snyppets/index.html#dbdict If api is restricted only to standard ones then people can't use their custom made containers.


On the one hand if they inherit from the base classes, there isn't an issue with this. Unfortunately in that case `DictMixin` and `UserDict` aren't subclasses of `dict`.


@shazow 
I meant the ones we are talking here :) Like checking for dict, list, set etc.


@piotr-dobrogost A big chunk of the results in @sigmavirus24's grep were for backwards compat. Another chunk was the base case reduction to 'str', which otherwise uses duck typing as you prefer. :) I suspect about half of the rest could be removed/improved.


Yep. I generally over step to see what happens first. I generally tinker a lot before getting things to where they should be and my commit message was probably misleading. (I count tinkering as work because I inevitably learn something.)


Again, I think this is pedantic. Please feel free to send small pull requests changing this behavior if you'd like. 


I'm going to send a PR just for the `register_hook` function. It's the only thing that needs to be done right now.


FYI, I wrote a helper which could be useful here, feel free to steal the code: https://github.com/shazow/unstdlib.py/pull/6


Which is similar to my version here: https://gist.github.com/3790156