Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add json parameter #2258

Merged
merged 5 commits into from Oct 5, 2014

Conversation

@sigmavirus24
Copy link
Contributor

commented Sep 30, 2014

Closes #2025

@sigmavirus24

This comment has been minimized.

Copy link

commented on requests/models.py in 8f17741 Aug 29, 2014

Sorry for the confusion, this section should be:

if data and _json is None:
    body = self._encode_params(data)
    if isintance(data, basestring) or hasattr(data, 'read'):
        content_type = None
    else:
        content_type = 'application/x-www-form-urlencoded'

@sigmavirus24 sigmavirus24 self-assigned this Sep 30, 2014

"""Sends a POST request. Returns :class:`Response` object.
:param url: URL for the new :class:`Request` object.
:param data: (optional) Dictionary, bytes, or file-like object to send in the body of the :class:`Request`.
:param json: (optional) json data to send in the body of the :class:`Request`.

This comment has been minimized.

Copy link
@kevinburke

kevinburke Sep 30, 2014

Contributor

This would get passed through with the kwargs, right, so I guess this is just so people know about the parameter?

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Author Contributor
@@ -190,6 +190,7 @@ class Request(RequestHooksMixin):
:param headers: dictionary of headers to send.
:param files: dictionary of {filename: fileobject} files to multipart upload.
:param data: the body to attach the request. If a dictionary is provided, form-encoding will take place.
:param json: json for the body to attach the request.

This comment has been minimized.

Copy link
@kevinburke

kevinburke Sep 30, 2014

Contributor

s/attach the request/attach to the request :)

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Author Contributor

Fixed

@@ -289,14 +294,14 @@ def __init__(self):
self.hooks = default_hooks()

def prepare(self, method=None, url=None, headers=None, files=None,
data=None, params=None, auth=None, cookies=None, hooks=None):
data=None, json=None, params=None, auth=None, cookies=None, hooks=None):

This comment has been minimized.

Copy link
@kevinburke

kevinburke Sep 30, 2014

Contributor

In the event that someone is calling prepare with all of its arguments and no keyword arguments, won't this suddenly change their params argument to now be the json argument?

Eg

request.prepare('get', '/foo', {}, {}, {}, {'foo': 'bar'}, {'auth': 'pass'})

Everything gets shifted by one.

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Author Contributor

Good point. This parameter should be moved to the end since we'll be invoking it as a keyword argument anyway.

One more reason I wish we were using Python 3, because then we could make these all keyword-only arguments or only some of them.

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Author Contributor

Not to say this is invalid, but I wonder how many people call prepare on a PreparedRequest.

This comment has been minimized.

Copy link
@Lukasa

Lukasa Sep 30, 2014

Member

Suspect quite a few. The real question is how many do it with positional arguments only. Regardless, let's not offer them a footgun.

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Author Contributor

Fixed

@sigmavirus24 sigmavirus24 changed the title Add json Add json parameter Sep 30, 2014

@@ -397,7 +402,7 @@ def prepare_headers(self, headers):
else:
self.headers = CaseInsensitiveDict()

def prepare_body(self, data, files):
def prepare_body(self, data, files, _json=None):

This comment has been minimized.

Copy link
@Lukasa

Lukasa Sep 30, 2014

Member

Why the underscore?

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Author Contributor

Working on changing that already. The problem is that the module json is imported at the top and needs to be used.

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Author Contributor

Fixed by aliasing json_dumps = json.dumps at the top of the module. Not sure I like it, but apparently import semantics are such that requests.compat.json can't be imported from (e.g., you can't do from .compat.json import dumps). Can you think of a better solution to avoid the local variable/parameter from shadowing the top-level module name beyond doing from . import compat and then doing compat.json.dumps?

Fix a couple of issues I noticed
- Don't _ prefix json in prepare_body
- Don't initialize json to []
- Don't initialize json to {}
- Reorder parameters to PreparedRequest.prepare
- Remove extra parentheses
- Update docstring
@sigmavirus24

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2014

@Lukasa @kevinburke care to take another look at this?

@Lukasa

This comment has been minimized.

Copy link
Member

commented Oct 5, 2014

LGTM

@kennethreitz

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2014

omg I'm so excited

❤️ 🍰 🌹 🍰 ❤️

kennethreitz added a commit that referenced this pull request Oct 5, 2014

@kennethreitz kennethreitz merged commit 1e79cf6 into psf:master Oct 5, 2014

@sigmavirus24

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2014

@willingc thank you so much for your hard work on this. 🍰 (Those emoji from @kennethreitz are for you ;))

@willingc

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2014

Thank you @sigmavirus24 for your support and mentoring 🎸 🌅 🌲 , @Lukasa for the review 🐱 🐈 😸 , and @kennethreitz for the community atmosphere 📷 ✌️ 🎸

@kennethreitz

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2014

@willingc thank YOU, and you're welcome :)

@@ -209,6 +212,7 @@ def __init__(self,
headers=None,
files=None,
data=None,
json=None,

This comment has been minimized.

Copy link
@kevinburke

kevinburke Oct 5, 2014

Contributor

Sorry I'm late to the party, was out - wouldn't this also change the meaning for anyone who creates a Request by hand, eg

params = {'date_created': '1-1-14'}
req = Request('GET', 'http://jsonip.com', {'User-Agent': 'foobar'}, None, {}, params)

where now suddenly params is JSON data?

@@ -376,6 +377,7 @@ def prepare_request(self, request):
def request(self, method, url,
params=None,
data=None,
json=None,

This comment has been minimized.

Copy link
@kevinburke

kevinburke Oct 5, 2014

Contributor

Same comment here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.