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

Made SimpleJson optional #182

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

baseplate-admin
Copy link

@baseplate-admin baseplate-admin commented Jan 24, 2023

Closes #181

@baseplate-admin baseplate-admin changed the title Fixes #181 Made SimpleJson optional Jan 24, 2023
Copy link
Member

@asfaltboy asfaltboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for slow response. Thanks for this PR, I do think it makes sense.

I think we can take it a step further and just remove simplejson entirely now. I say this because it seems we only used simplejson to support python 2.7/3.3, as per their docs:

simplejson exposes an API familiar to users of the standard library marshal and pickle modules. It is the externally maintained version of the json library, but maintains compatibility with the latest Python 3.8+ releases back to Python 3.3 as well as the legacy Python 2.5 - Python 2.7 releases.

Before we apply merge the change, though, could we add a test case or two to advanced_filters/tests/test_q_serializer.py which ensure serialising and unserialising date and datetime values work?

@baseplate-admin
Copy link
Author

Hi, Thanks for your response. I am a bit busy now. Will swing back to this PR soon.


I think we can take it a step further and just remove simplejson entirely now. I say this because it seems we only used simplejson to support python 2.7/3.3, as per their docs:

I also think stdlib's json does make more sense to support.

Before we apply merge the change, though, could we add a test case or two to advanced_filters/tests/test_q_serializer.py which ensure serialising and unserialising date and datetime values work?

Sure gimme some time

@baseplate-admin
Copy link
Author

Hi, sorry i forgot about this PR.

#182 (review) has been addressed. Please take a look.

Update setup.py
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.

Make SimpleJSON optional
2 participants