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

Regression in 1.4.1 #145

Merged
merged 1 commit into from
Feb 29, 2016
Merged

Regression in 1.4.1 #145

merged 1 commit into from
Feb 29, 2016

Conversation

prokaktus
Copy link
Contributor

This PR addressed to #144.

Extending JSONEncoder:
https://docs.python.org/2/library/json.html
https://docs.python.org/3.6/library/json.html

Yeah, I understand that Decimal and float are not the same, but we have a trade-offs in this case:

  • explicitly require simplejson (to allow consistent behaviour on different setups and test passing)
  • more low-level json.JSONEncoder redefinition, but it may breaks on future changes. Also, I'm not looking into that and don't know, how it should be implemented
  • or convert Decimals to float and leave some representation problems with numbers like Decimal('3.93999999999999999'). After json.dumps it would look like 3.94.

I've chosen third.

Also I cleaned some old code, that I've met. I'll be glad to listen any ideas about implementation and realization, if you have!

@prokaktus prokaktus closed this Feb 11, 2016
@prokaktus prokaktus reopened this Feb 11, 2016
@prokaktus
Copy link
Contributor Author

Hm-m-m. It has red CI, with errors from psycopg2 psycopg2.InterfaceError: connection already closed.

I don't understand why this is happening. Try to investigate later.

@nemesifier
Copy link
Member

@prokaktus you should not try to fix two issues at the same time, please send a separate pull request to drop unsupported django versions.

@prokaktus
Copy link
Contributor Author

I'm still have errors InterfaceError: connection already closed, And as I can see from build logs, It's already happened earlier https://travis-ci.org/djangonauts/django-hstore/builds/88919164

It's always happens in the same configurations in pull-request, but travis-ci in my fork repo is green. Looks pretty strange..

@prokaktus
Copy link
Contributor Author

Please do not review PR yet. I cannot understand why sometimes tests passing and why sometimes not. I see some correlations, but I cannot describe it. Right now commits contains a lot of trash and manipulation with tests. I mention when you it will be done (I hope that...).

@prokaktus prokaktus force-pushed the master branch 4 times, most recently from ed5d194 to dca9509 Compare February 16, 2016 00:47
@prokaktus
Copy link
Contributor Author

@nemesisdesign I cannot figure out what exactly is going wrong with tests. My travis tests passing good
https://travis-ci.org/prokaktus/django-hstore/builds/109494463

But pull requests always failing in the same configurations (py3.3-django1.8, py2.7-django1.8, py2.7-django1.9). This is failing example
https://travis-ci.org/djangonauts/django-hstore/builds/109215630

After some tests postgres just closing connection. I've tried to add connection.connect() on this exception, but without any luck.

Because this is already happens earlier, I think that maybe there exists some workaround. Similar problem already happens in #133 https://travis-ci.org/djangonauts/django-hstore/builds/88919938 and #128 https://travis-ci.org/djangonauts/django-hstore/builds/73041973

But PR discussion doesn't shed light on how exactly tests problems were fixed. And right now I don't understand what I can do with it.

Thanks for your attention!

@nemesifier
Copy link
Member

It may be because of this django-bug: https://code.djangoproject.com/ticket/15802 which has been closed and reopened as https://code.djangoproject.com/ticket/24810. But I'm not sure.
On which python/django versions have you tested?

@nemesifier
Copy link
Member

Or it could also be this one: travis-ci/travis-ci#1125

I tested your PR on django 1.9 and python 3.4 and everything is ok. I'll take a look at the code asap and if everything is ok we will merge even if the build is failing.

PS: I also tried restarting the build a few times with no luck

@prokaktus
Copy link
Contributor Author

@nemesisdesign

On which python/django versions have you tested?

I've tested in my travis:
https://travis-ci.org/prokaktus/django-hstore/builds/109494463
All green.

On local machine, I've tested python 3.5/Django 1.9 and with Python 2.7/Django 1.8 (one of the failing configuration in Travis), but with no luck: tests passing.

I thought, that if this already happened (see links above), maybe exists some workaround. But I cannot find any specific commits or discussions.

nemesifier added a commit that referenced this pull request Feb 29, 2016

Verified

This commit was signed with the committer’s verified signature.
1.4.1 regression: TypeError: Decimal is not JSON serializable
@nemesifier nemesifier merged commit c7d3c36 into djangonauts:master Feb 29, 2016
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

Successfully merging this pull request may close these issues.

None yet

2 participants