Fixing error return to be consistent with tuple return at the end of the method

In passing I managed to produce a situation where I would get an error when this check fails. When this gets called on line 391:

``` python
391            # Multi-part file uploads.
392            if files:
393                (body, content_type) = self._encode_files(files, data)
```

Which would result in:
`TypeError: 'NoneType' object is not iterable`

This patch fixes that return.


Can you provide a snippet of code that reproduces the bug in the current version?


Ok, I dug into this a little more and figured out exactly what the cause is. When you pass an string (or unicode) as the data param and a files dict to requests.post(), it returns None at the place my patch changes behavior.  The downside of fixing this is that now when you pass a string body (empty or not) and a files dict, requests won't raise any sort of indication that it's ignoring your files dict. Here are three scenarios, you can just run this with nose to see the errors.

``` python
#! /usr/bin/env python
import requests
import StringIO


def test_regular_string_post():
    # Silently succeeds, but doesn't tell you that requests is ignoring your files dict
    data = ""
    stream = StringIO.StringIO("some content goes here")
    resp = requests.post('http://httpbin.org/post', data=data,
                         headers={'Accept': u'*/*', 'Host': u'httpbin.org', 'Expect': u'100-continue'},
                         files={'file': (u'file.txt', stream, 'application/octet-stream')},
                         cookies={},
                         timeout=120)
    print resp.content


def test_unicode_string_post():
    # Loudly fails with TypeError
    data = u""
    stream = StringIO.StringIO("some content goes here")
    resp = requests.post('http://httpbin.org/post', data=data,
                         headers={'Accept': u'*/*', 'Host': u'httpbin.org', 'Expect': u'100-continue'},
                         files={'file': (u'file.txt', stream, 'application/octet-stream')},
                         cookies={},
                         timeout=120)
    print resp.content


def test_none_post():
    # Correctly succeeds
    data = None
    stream = StringIO.StringIO("some content goes here")
    resp = requests.post('http://httpbin.org/post', data=data,
                         headers={'Accept': u'*/*', 'Host': u'httpbin.org', 'Expect': u'100-continue'},
                         files={'file': (u'file.txt', stream, 'application/octet-stream')},
                         cookies={},
                         timeout=120)
    print resp.content

```


This brings up a separate question, should _encode_files() raise an error when a files dict and a data string are provided rather than ignoring the files dict and posting the body?


@skastel I think the answer here is yes. Here's why:

If the user passes the `files` parameter, Requests will _always_ do a `multipart/form-data` upload. Providing a simple plain text string as well doesn't easily work alongside that kind of upload, at least not in the current API. For this reason, I think the user should probably get a `ValueError` explaining that `data` may not be a string if the `files` parameter is present.


I agree but that effectively changes the API in a way that may seem (although I find it hard to argue that it actually is) backwards incompatible. Would it then make sense to only raise that exception in 2.0?


Ah, yeah, I'm implicitly viewing all current feature requests as 2.0 feature requests. =)


I should clarify that I'm not opposed to including this for 2.0.


@Lukasa I'd actually much prefer a `ValueError` here, it would be clear that something is wrong with the request rather than making the request without uploading the files. So rather than silently "succeeding" it would explicitly fail and the raised error could indicate that a `data` was a string instead of a dict. I agree it changes the API, but I don't think this is part of the documented API, is it?


@skastel part of the documented API or not, it's a behaviour some people may (possibly unintentionally) rely on. People do weird things with code you release and they don't always tell you until you change it. This is a backwards incompatible change either way you slice it unfortunately.


@sigmavirus24 is right. We're fascistic about the API. It should be set in stone outside of major version increases. Happily, 2.0 isn't too far away, so this is well timed.


Sounds reasonable. For now this pull should at least result in consistent behavior whether data is a `unicode` or a `str` 


@Lukasa recommendation?


I'm strongly +1 on changing this to throw an explanatory `ValueError`. If @skastel is happy to make that change, I'll be totally happy with merging this. =)


Hey, sorry for the slow reply, just got back from a vacation.

So would we prefer to have models.py be modified to throw a `ValueError` like so:

```
95        if (not files):
96            return (None, None)
97        if isinstance(data, basestring):
98            raise ValueError("Can't execute command with data string and files dict
```

Does that seem like the right change?


I think that where we currently return `None` we should instead throw a `ValueError`. One of the following two behaviours:

``` python
if (not files) or isinstance(data, basestring):
    raise ValueError("Invalid argument combination.")
```

``` python
if (not files):
    raise ValueError("Files must be provided.")
elif isinstance(data, basestring):
    raise ValueError("Data must not be a string.")
```


Latest commit will raise `ValueError` when we get the inconsistent data/files arguments.


Awesome. For the testing, take a look at [this](http://pytest.org/latest/assert.html#assertions-about-expected-exceptions).


Tests use `self.assertRaises()` now instead of just running the code. Thanks for the tip!


Ah, sorry, I should have been clearer. You can't use the context manager form of `assertRaises()` on Pythons earlier than 2.7. You'll need to use one of these two forms:

``` python
pytest.raises(ExpectedException, func, *args, **kwargs)
pytest.raises(ExpectedException, "func(*args, **kwargs)")
```

This is why the tests failed. That's totally my bad, sorry. =(


Oh, d'oh, ok I'll fix that up.


All clear now!


Awesome, this is great work! =D I'm +1 on merging this for 2.0. 


Great, anything else you need from me, or should I just leave this open until you're ready to merge it into 2.0?


As far as this PR goes, your work is done! Kenneth is the BDFL so he's the only person who merges code to this repo, so this will sit here until he has time to look at it. =) But rest assured, it _will_ get merged (or I'll annoy him until he does it).

Again, thanks so much! =D :cookie: :star: :cookie: