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 datetime.date support to JSONEncoder #1326

Merged
merged 1 commit into from Jan 23, 2015

Conversation

@RealSalmon
Copy link
Contributor

commented Jan 22, 2015

Add support for the datetime.date type to jsonify, which currently only supports the datetime.datetime type.

Changelog
  1. datetime.date type support in flask.json.jsonify
@davidism

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

datetime is a subclass of date, there's no need to check both. Simply isinstance(o, date) will suffice.

@RealSalmon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2015

@davidism - thanks! I've reduced it to a single check.

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

You should add a test for this as well. In addition, the commits can be squashed into one.

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

And please add a changelog too :)

You can squash commits with:

git rebase -i master

But it doesn't really matter if you don't.

@davidism Thanks for reviewing this!

@RealSalmon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2015

Will do, thanks!

@RealSalmon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2015

Added tests for both datetime.datetime and datetime.date types.

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

Could you add a changelog item for 1.0 too?

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2015

why does the "unit-test" involve a flask app and a test client, its just jsonify this time
(i realize the surrounding tests are acceptance tests of request handling)

@RealSalmon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2015

@untitaker - changelog entry added. sorry - I misunderstood what you were asking for before.

@RonnyPfannschmidt - mainly it's that way because that's how all of other tests are to be set up, but also because jsonify doesn't seem to work outside of an application context. I supposed because it returns an actual response in addition to the serialized data.

@untitaker untitaker merged commit 61263e0 into pallets:master Jan 23, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

untitaker added a commit that referenced this pull request Jan 23, 2015

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

Woo, thanks!

Your explanation for using a temporary app for testing is sound, although I haven't explored this myself.

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