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

BetterJSONEncoder in tests is not simplejson #583

Closed
Natureshadow opened this issue Sep 16, 2016 · 6 comments · Fixed by #584
Closed

BetterJSONEncoder in tests is not simplejson #583

Natureshadow opened this issue Sep 16, 2016 · 6 comments · Fixed by #584
Labels

Comments

@Natureshadow
Copy link
Contributor

The test suites fail because the JSONEncoder that is extended to BetterJSONEncoder in tests/helpers.py is not from simplejson, whereas the JSONEncoder used by Flask is.

Changing "from json import JSONEncoder to" "from simplejson import JSONEncoder" (and probably explicitly depending on simplejson) solves the issue.

@jfinkels
Copy link
Owner

Thanks for reporting this! I'd rather inherit the json implementation from Flask if at all possible, so Flask-Restless doesn't have to check whether simplejson/json can be imported. I have done this in the code where you see from flask import json. Can we do from flask.json import JSONEncoder?

@jfinkels jfinkels added the bug label Sep 16, 2016
@Natureshadow
Copy link
Contributor Author

I'll shortly do a pull request ☺

@jfinkels
Copy link
Owner

It may be worth checking whether there are any other places in the code where we import from json instead of from flask import json.

@Natureshadow
Copy link
Contributor Author

I did check that ☺.

@Natureshadow
Copy link
Contributor Author

Oh, but now I see - in the tests, we could also use the JSONEncoder already imported by flask.json instead of doing the check for simplejson on our own, right?

@Natureshadow
Copy link
Contributor Author

Natureshadow commented Sep 16, 2016

OK, simply using flask.json.JSONEncoder for feriving BetterJSONEncoder indeed does the trick.

Note that I mixed in another fix, because the .helpers module uses sys without importing it.

EDIT: The last sentence was never spoken. Really. I should not be fixing code after too many hours of work ;)!

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

Successfully merging a pull request may close this issue.

2 participants