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

ajax_request decorator - use django json encoder #16

Open
cezio opened this issue Jan 6, 2014 · 22 comments
Open

ajax_request decorator - use django json encoder #16

cezio opened this issue Jan 6, 2014 · 22 comments

Comments

@cezio
Copy link
Contributor

cezio commented Jan 6, 2014

Current json serializer doesn't support few basic python types. Instead of half-baked serializer used now, maybe Django's encoder class should be used:

https://github.com/django/django/blob/master/django/core/serializers/json.py#L84

This encoder supports additional types:

  • datetime,
  • date,
  • time,
  • decimal
@skorokithakis
Copy link
Owner

That's a good idea, do you see anything breaking from changing serializers? What about people who expected an exception and implemented their own serializers instead, will those break?

@xblitz
Copy link

xblitz commented Apr 2, 2014

+1 on this... encoding Decimals is a must for me..

@skorokithakis
Copy link
Owner

This is possible now. You can add into your settings:

FORMAT_TYPES = {
    'application/json': lambda response: json.dumps(response, default=date_time_handler),
    'text/json':        lambda response: json.dumps(response, default=date_time_handler),
}

And use the serializer you want, including Django's. Can someone add a (tested) example in the docs so I can close this issue?

@pawelnowak
Copy link

Slightly off-topic, but could you explain why the 0.7.9 version of django-annoying available through pypi (https://pypi.python.org/pypi/django-annoying/0.7.9) differs from the 0.7.9 version of code available here, on github? Don't they periodically refresh packages in the index?

As a result of that when you install django-annoying using pip you get an older version of the package that, among other things, lacks the support of settings-level customization of the FORMAT_TYPES dictionary.

@skorokithakis
Copy link
Owner

That's very odd, you are right. It does differ. Maybe I forgot to bump the version? I will make a new release, thanks for the heads up.

@cezio
Copy link
Contributor Author

cezio commented Jul 8, 2014

#16 (comment)

I believe point of django-annoying is to eliminate certain annoyances in Django framework;) Need of specifying additional settings (that would be a hundredth something setting in a project) doesn't seem to be aligned with the goal above for me.

@skorokithakis
Copy link
Owner

@cezio, sure, but do you have a better idea?

@skorokithakis
Copy link
Owner

My concern here is that it will break things for current users. I do agree that the better serializer should be used by default...

@mknecht
Copy link

mknecht commented Apr 10, 2015

How about the usual approach of creating a new decorator, and deprecating the old one?

(edited because mistyped, and sent too early)

@skorokithakis
Copy link
Owner

Hmm, that's an idea, but the new decorator would necessarily be misnamed a bit, so I'm not a huge fan of this practice...

@mknecht
Copy link

mknecht commented Apr 10, 2015

True. How about a configuration option that defaults to the old behavior:

@ajax_request(use_django_encoder=True)
def cost_view(request):
    return {'total': Decimal("4")}

@ajax_request()
def user_view(request):
    return {'id': 7}

Arguably, though, it's misnamed already — at least it confused me, since AJAX is just one particular way for a client to access the API. This decorator is really for API views, providing machine-readable representations of a resource, no? I.e. api_view or api_response might not be a bad fit. Or, more behavior-related names: encode_response, encode_return_value, … well, just some thoughts. :)

@skorokithakis
Copy link
Owner

The extra parameter still breaks backwards compatibility, since it's not called with parameters by default, but I really like the alternative names. Something like json_response might be the most fitting, being very literal.

@mknecht
Copy link

mknecht commented Apr 10, 2015

But it does support YAML, too, no? That's why I shed away from the JSON name. :)

Also, Decorators that can be applied with and without arguments are possible, i.e. you don't have to break backwards-compatibility. You “merely” need to consider the type of the first argument, if it's a function or not. If you always use keyword args for the arguments, then it would be perfectly safe, too. Might not be the most beautiful solution on your side of the code, but if you don't want to change the name, and still fix this issue, you can. :)

@skorokithakis
Copy link
Owner

Yeah, you're right. I didn't consider that we'd never be accepting function arguments in normal usage, so I'll go with your proposed fix above. If anyone wants to beat me to a PR, you are very welcome to.

@mknecht
Copy link

mknecht commented Apr 11, 2015

Just had to serialize something to JSON, and wanted to try out the built-in serializer. Turns out it is restricted to Django models. Also, a serialize-deserialize does not yield the same objects: For example, decimals stay a string.

So, I don't think this annotation should be switching to Django's serializer. At least I use API views for more than just plain Django models. If I'm missing something, do tell. :)

@skorokithakis
Copy link
Owner

@mknecht, have you found something that the default json module will serialize but Django's JSON encoder won't? Decimals do stay a string, but the default JSON encoder produces an exception, so I'm not sure that's a valid comparison.

@mknecht
Copy link

mknecht commented Apr 11, 2015

@skorokithakis It appears that the serializer will only serialize Django models. But, I would like to use the decorator for primitive object graphs, too, like a dict.

The linked documentation indicates that, and I tried and it refused to serialize dicts and such because they did not have a _meta attribute.

@skorokithakis
Copy link
Owner

That's odd, I've been using it for proper serialization of dates and it's worked fine. I'll investigate.

@mknecht
Copy link

mknecht commented Apr 11, 2015

Maybe I misunderstood something. Thought it odd, too. I'd be glad to hear it works / what I did wrong. :)

@mknecht
Copy link

mknecht commented Apr 12, 2015

No, you're right, works just fine. I must have messed something up. Sorry for the confusion. :)

@troyleesmith
Copy link

Yo is it possible to have this issue closed up?

@skorokithakis
Copy link
Owner

Certainly, PRs are welcome. I am a bit swamped right now :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants