Repository: MechanicalSoup/MechanicalSoup

Issue:
Unexpected keyword argument 'proxies'
The use of requests.Request base class does not include a proxies keyword like the requests.request class.

```python
import requests
import mechanicalsoup
self.Session = requests.Session()
self.browser = mechanicalsoup.StatefulBrowser(session=self.Session)
self.browser.get(searchURL.url, proxies=self.proxy)
```

Full error log is:

```
    response = self.browser.submit_selected(proxies=self.proxy)
  File "C:\Users\Kevin\Desktop\Project\dev\lib\site-packages\mechanicalsoup\stateful_browser.py", line 194, in submit_selected
    *args, **kwargs)
  File "C:\Users\Kevin\Desktop\Project\dev\lib\site-packages\mechanicalsoup\browser.py", line 209, in submit
    request = self._prepare_request(form, url, **kwargs)
  File "C:\Users\Kevin\Desktop\Project\dev\lib\site-packages\mechanicalsoup\browser.py", line 191, in _prepare_request
    request = self._build_request(form, url, **kwargs)
  File "C:\Users\Kevin\Desktop\Project\dev\lib\site-packages\mechanicalsoup\browser.py", line 188, in _build_request
    return requests.Request(method, url, files=files, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'proxies'
```

Discussion:
moy: Don't forget to update the changelog.

I'm wondering whether we're loosing some potential flexibility by doing this. Currently, we're not, but what if we ever want to provide a hook that the user could execute between the "prepare" and the "send" step? Will it still be possible?
hemberger: ChangeLog has been updated.

Your question is a good one. From the current version of the code, we're only gaining generality. Since request construction and sending was all done internally in `submit`, even without this PR we would need to change the API to allow users to modify the prepared request before sending. I don't think this should be any harder of a task after this PR is merged. However, it might never be unnecessary -- I think the idea behind `requests.Sessions.request` is to allow people to specify almost anything you'd need to do for both the preparing and sending in a single call.
moy: > without this PR we would need to change the API to allow users to modify the prepared request before sending

Yes, we would need to modify our API, but it's rather simple to provide a `add_hook(callback)` and to call the callback between prepare and send before the PR.

> I don't think this should be any harder of a task after this PR is merged.

The problem is that the lines of code between prepare and send are currently in our code, so we can still add whatever we want (like calling a user-provided callback) there. After the PR, the only lines of code between prepare and send are in the requests library, we can't modify them anymore.

>  I think the idea behind requests.Sessions.request is to allow people to specify almost anything

Yes, but the question is whether this "almost" is sufficient for 100% use-cases. The request library provides the flexibility of allowing tweaking the prepared request before sending:

http://docs.python-requests.org/en/master/user/advanced/#prepared-requests

We don't currently need it, but it'd be nice to keep the possibility to do it in the future.

Re-reading the code, I think there's another way to expose the full flexibility of the requests library: merge your PR and then split `_request` into:

* `_extract_form_data()` that would build `kwargs` (i.e. the current `_request()`, except its last line)
* `_send_form_data()` that would call `self.session.request(..., kwargs)`

(Or perhaps these would deserve a name not starting with `_`)

and expose `_request()` that would just call these two functions. This way, the user may, if needed, call `_extract_form_data()` and do whatever tweaking is needed to actually send the HTTP request. This would slightly increase the flexibility of the code, and would also split the form-related code and the http-related code better.

I short, I think we can merge the PR, and we may want to do the split I suggest later.
hemberger: Yep, that split sounds completely natural to me. The problem with the previous split was that it combined `prepare` and `send`, but had no way of conveniently passing any parameters to `send`.

Patch:
diff --git a/docs/ChangeLog.rst b/docs/ChangeLog.rst
index 9b61e30..472b8e1 100644
--- a/docs/ChangeLog.rst
+++ b/docs/ChangeLog.rst
@@ -5,6 +5,19 @@ Release Notes
 Version 1.0 (in development)
 ============================
 
+Main changes:
+-------------
+* ``Browser.submit`` and ``StatefulBrowser.submit_selected`` accept a larger
+  number of keyword arguments. Arguments are forwarded to
+  `requests.Session.request <http://docs.python-requests.org/en/master/api/#requests.Session.request>`__.
+  [`#166 <https://github.com/MechanicalSoup/MechanicalSoup/pull/166>`__]
+
+Internal changes:
+-----------------
+* Private methods ``Browser._build_request`` and ``Browser._prepare_request``
+  have been replaced by a single method ``Browser._request``.
+  [`#166 <https://github.com/MechanicalSoup/MechanicalSoup/pull/166>`__]
+
 Version 0.9
 ===========
 
diff --git a/mechanicalsoup/browser.py b/mechanicalsoup/browser.py
index 9c5e2f4..02513f9 100644
--- a/mechanicalsoup/browser.py
+++ b/mechanicalsoup/browser.py
@@ -121,7 +121,7 @@ class Browser(object):
         Browser.add_soup(response, self.soup_config)
         return response
 
-    def _build_request(self, form, url=None, **kwargs):
+    def _request(self, form, url=None, **kwargs):
         method = str(form.get("method", "get"))
         action = form.get("action")
         url = urllib.parse.urljoin(url, action)
@@ -185,11 +185,7 @@ class Browser(object):
         else:
             kwargs["data"] = data
 
-        return requests.Request(method, url, files=files, **kwargs)
-
-    def _prepare_request(self, form, url=None, **kwargs):
-        request = self._build_request(form, url, **kwargs)
-        return self.session.prepare_request(request)
+        return self.session.request(method, url, files=files, **kwargs)
 
     def submit(self, form, url=None, **kwargs):
         """Prepares and sends a form request.
@@ -197,8 +193,8 @@ class Browser(object):
         :param form: The filled-out form.
         :param url: URL of the page the form is on. If the form action is a
             relative path, then this must be specified.
-        :param \*\*kwargs: Arguments forwarded to `requests.Request
-            <http://docs.python-requests.org/en/master/api/#requests.Request>`__.
+        :param \*\*kwargs: Arguments forwarded to `requests.Session.request
+            <http://docs.python-requests.org/en/master/api/#requests.Session.request>`__.
 
         :return: `requests.Response
             <http://docs.python-requests.org/en/master/api/#requests.Response>`__
@@ -206,8 +202,7 @@ class Browser(object):
         """
         if isinstance(form, Form):
             form = form.form
-        request = self._prepare_request(form, url, **kwargs)
-        response = self.session.send(request)
+        response = self._request(form, url, **kwargs)
         Browser.add_soup(response, self.soup_config)
         return response
 


Test patch:
diff --git a/tests/test_browser.py b/tests/test_browser.py
index 2bb9192..1ff9871 100644
--- a/tests/test_browser.py
+++ b/tests/test_browser.py
@@ -64,26 +64,26 @@ form_html = """
 """
 
 
-def test_build_request():
+def test__request():
     form = BeautifulSoup(form_html, "lxml").form
 
     browser = mechanicalsoup.Browser()
-    request = browser._build_request(form)
+    response = browser._request(form)
 
-    assert request.data["customer"] == "Philip J. Fry"
-    assert request.data["telephone"] == "555"
-    assert request.data["comments"] == "freezer"
-    assert request.data["size"] == "medium"
-    assert request.data["topping"] == ["cheese", "onion"]
-    assert request.data["shape"] == "square"
+    data = response.json()['form']
+    assert data["customer"] == "Philip J. Fry"
+    assert data["telephone"] == "555"
+    assert data["comments"] == "freezer"
+    assert data["size"] == "medium"
+    assert data["topping"] == ["cheese", "onion"]
+    assert data["shape"] == "square"
 
-    request = browser._prepare_request(form)
-    assert "application/x-www-form-urlencoded" in request.headers[
+    assert "application/x-www-form-urlencoded" in response.request.headers[
         "Content-Type"]
     browser.close()
 
 
-def test_prepare_request_file():
+def test__request_file():
     form = BeautifulSoup(form_html, "lxml").form
 
     # create a temporary file for testing file upload
@@ -94,15 +94,16 @@ def test_prepare_request_file():
     form.find("input", {"name": "pic"})["value"] = pic_path
 
     browser = mechanicalsoup.Browser()
-    request = browser._build_request(form)
+    response = browser._request(form)
 
-    assert request.files
-    assert request.files["pic"]
-    assert request.files["pic"].read() == ":-)".encode("utf8")
-    assert "pic" not in request.data
+    # Check that only "files" includes a "pic" keyword in the response
+    for key, value in response.json().items():
+        if key == "files":
+            assert value["pic"] == ":-)"
+        else:
+            assert (value is None) or ("pic" not in value)
 
-    request = browser._prepare_request(form)
-    assert "multipart/form-data" in request.headers["Content-Type"]
+    assert "multipart/form-data" in response.request.headers["Content-Type"]
     browser.close()