ColtonProvias / sqlalchemy-jsonapi

JSONAPI implementation for use with SQLAlchemy
MIT License
71 stars 27 forks source link

Support Decimal type in JSON serializer #25

Closed jimbobhickville closed 7 years ago

jimbobhickville commented 8 years ago

SQLa Numeric types get returned as decimal.Decimal objects. simplejson has support for this, whereas the builtin json module does not and there's no clean way to subclass it in a way that it won't convert the value to a JSON string.

Fixes #23

jimbobhickville commented 8 years ago

@ColtonProvias - any chance you'll have time to review these PRs soon?

Anderycks commented 7 years ago

@jimbobhickville I'm lending a hand now and helping Colton get PRs reviewed. I'll be taking a look off and on through this week. More feedback coming soon.

Anderycks commented 7 years ago

What happens in the builtin json module with Decimal types? Does it just throw an exception?

ColtonProvias commented 7 years ago

It does throw an exception if I recall. Just have to extend the JSONEncoder class appropriately to fix.

On Fri, Feb 24, 2017 at 12:55 PM Deryck Hodge notifications@github.com wrote:

What happens in the builtin json module with Decimal types? Does it just throw an exception?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ColtonProvias/sqlalchemy-jsonapi/pull/25#issuecomment-282358353, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQqHj4Z69mKLK7_HOXgjWmX9WVaa2eUks5rfxmGgaJpZM4JuwOT .

-- Colton J. Provias Film Composer and Audio Engineer Chief Technology Officer, Zeitghost Technologies +1.330.776.8427

jimbobhickville commented 7 years ago

Yep, it throws an exception. I first attempted to override the builtin, but you'd have to monkey-patch a pretty critical method to override the behavior iirc. Always possible I missed an easier solution.

Anderycks commented 7 years ago

OK, makes sense. I'd prefer to fix this the way Colton suggests -- patching our JSONAPIEncoder class, rather than taking on a new dependency on simplejson. I understand if that's more work than you'd like to take on, though @jimbobhickville.

jimbobhickville commented 7 years ago

If I get back to the project that was using this, I'll work on a new patch.

Anderycks commented 7 years ago

@jimbobhickville Sounds good. Thanks!