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

Projects
None yet
5 participants
@sigmavirus24
Member

sigmavirus24 commented Sep 30, 2014

Closes #2025

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Aug 29, 2014

Collaborator

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'
Collaborator

sigmavirus24 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.

@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?

@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.

@sigmavirus24
Show outdated Hide outdated requests/models.py
@@ -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.

@kevinburke

kevinburke Sep 30, 2014

Contributor

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

@kevinburke

kevinburke Sep 30, 2014

Contributor

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

This comment has been minimized.

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

Fixed

@sigmavirus24
Show outdated Hide outdated requests/models.py
@@ -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.

@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.

@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.

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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.

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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.

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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

This comment has been minimized.

@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.

@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.

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

Fixed

@sigmavirus24

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

Show outdated Hide outdated requests/models.py
@@ -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.

@Lukasa

Lukasa Sep 30, 2014

Member

Why the underscore?

@Lukasa

Lukasa Sep 30, 2014

Member

Why the underscore?

This comment has been minimized.

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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.

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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?

@sigmavirus24

sigmavirus24 Sep 30, 2014

Member

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.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Oct 4, 2014

Member

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

Member

sigmavirus24 commented Oct 4, 2014

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

@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Oct 5, 2014

Member

LGTM

Member

Lukasa commented Oct 5, 2014

LGTM

@kennethreitz

This comment has been minimized.

Show comment
Hide comment
@kennethreitz

kennethreitz Oct 5, 2014

Member

omg I'm so excited

❤️ 🍰 🌹 🍰 ❤️

Member

kennethreitz 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 requests:master Oct 5, 2014

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Oct 5, 2014

Member

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

Member

sigmavirus24 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.

Show comment
Hide comment
@willingc

willingc Oct 5, 2014

Contributor

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

Contributor

willingc 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.

Show comment
Hide comment
@kennethreitz

kennethreitz Oct 5, 2014

Member

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

Member

kennethreitz 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.

@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?

@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.

@kevinburke

kevinburke Oct 5, 2014

Contributor

Same comment here.

@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