Add json_dumps_kwargs in JSONResponseMixin #43

Merged
merged 5 commits into from Apr 15, 2013

Projects

None yet

3 participants

@brmzkw
brmzkw commented Apr 10, 2013

In my case, I need to output with indent=2. It is obviously possible to override render_json_response() completely, but it is more elegant to have a property set to the desired value.

Note that currently, ensure_ascii is set to True. This patch makes possible to set it to False (and I've no idea about why someone would do such a thing).

@chrisjones-brack3t
Member

Couple of issues I have with this.

  1. The class attribute json_dumps_kwargs must be a dictionary, however there is no type checking to make sure that is what was set. If I set it as a tuple or an integer, what will happen?
  2. The json.dumps command has a lot of optional args. Rather than just allowing a dict to be set, I'm wondering if handling most of the args wouldn't be a better idea. Not sure which way to go on that. By handling args I mean, maybe you just set json_indent as a class attribute with a default value and have a get_json_indent method which can be overridden should it need to be set dynamically. Same goes for most of the other optional args. http://docs.python.org/2/library/json.html#basic-usage
  3. Tests. I don't see any.
@kennethlove kennethlove commented on the diff Apr 10, 2013
braces/views.py
@@ -427,13 +428,19 @@ def get_content_type(self):
})
return self.content_type
+ def get_json_dumps_kwargs(self):
+ if self.json_dumps_kwargs is None:
+ self.json_dumps_kwargs = {}
+ self.json_dumps_kwargs.setdefault('ensure_ascii', False)
@kennethlove
kennethlove Apr 10, 2013 Member

Doesn't this actually set ensure_ascii to False unless someone explicitly sets it to True?

"This patch makes possible to set it to False (and I've no idea about why someone would do such a thing)."

If it currently is automatically set to True, we should ensure that continues.

@brmzkw
brmzkw Apr 10, 2013

You're right, I messed up. Here, I meant True and not False :-)

@brmzkw
brmzkw Apr 15, 2013

I took a look and it doesn't actually break anything. Currently, ensure_ascii is always set to False, not to True as you pointed it.
https://github.com/brack3t/django-braces/blob/master/braces/views.py#L436

Did I miss something?

@brmzkw
brmzkw commented Apr 10, 2013

Response to chrisjones:

The class attribute json_dumps_kwargs must be a dictionary, however there is
no type checking to make sure that is what was set. If I set it as a tuple or
an integer, what will happen?

The same thing is gonna happen if you set a dict to
JSONResponseMixin.content_type: probably not what you expect.

It's not really a problem to me. The documentation explains how to use the
attribute.

Feel free to insist in case you believe it's important, I'll do add the check.

The json.dumps command has a lot of optional args. Rather than just allowing
a dict to be set, I'm wondering if handling most of the args wouldn't be a
better idea. Not sure which way to go on that. By handling args I mean, maybe
you just set json_indent as a class attribute with a default value and have a
get_json_indent method which can be overridden should it need to be set
dynamically. Same goes for most of the other optional args.

I thought about this approach and I don't like it, because it means duplicating
the code from the json std module to the mixin.

Let's say that someday, json.dumps() method evolves and takes a new argument.
You would have to modify the mixin according to the new standard. Of course, it
will probably never happen. But I'm not comfortable with duplicating code.
Moreover, the code will get harder to follow given the huge of (unnecessary?)
amount of methods.

What do you think?

Tests. I don't see any.

Thanks for pointing this out, 'will fix that soon.

Julien Castets added some commits Apr 15, 2013
@brmzkw
brmzkw commented Apr 15, 2013

I created some tests. To be sure that json_dumps_kwargs is used, two requests are made to the same URL but with a different json_dumps_kwargs['indent'] value. If JSON response are the same but have a different length, I consider that everything is okay. I couldn't find a more straightforward check.

@chrisjones-brack3t chrisjones-brack3t merged commit aad651b into brack3t:master Apr 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment