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

Remove DemJSON Test #374

Merged
merged 7 commits into from Jan 9, 2022
Merged

Remove DemJSON Test #374

merged 7 commits into from Jan 9, 2022

Conversation

Theelx
Copy link
Contributor

@Theelx Theelx commented Jan 7, 2022

DemJSON doesn't support CPython 3.9+. It'd be worth considering dropping its backend, as DemJSON itself hasn't been updated since 2015.

DemJSON doesn't support CPython 3.9+. It'd be worth considering dropping its backend, as DemJSON itself hasn't been updated since 2015.
Globals should be avoided usually, but unittest doesn't seem to let us access nn-globals in the skipIf decorator.
Apparently unittest lets you skip conditionally inside the test itself!
Apparently some test cases don't call set_backend, so check if it's been called first.
I guess set_backend isn't called, it's always set_preferred_backend???
DemJSON hasn't been updated since December 2015, which is 6 years ago now. It should be safe to drop support, especially since there are many other faster backends supported, like ujson.
@Theelx
Copy link
Contributor Author

Theelx commented Jan 7, 2022

@davvid I'm in the process of fixing up all the tests and removing obsolete code. Is it okay to remove the DemJSON backend considering its last update date? Next, I plan to work on fixing the FutureWarning for the pandas tests, and fixing some DeprecationWarnings for the numpy dtype tests.

@Theelx Theelx changed the title Skip test when using DemJSON Remove DemJSON Test Jan 7, 2022
@Theelx
Copy link
Contributor Author

Theelx commented Jan 9, 2022

I'm going to merge this, and hopefully fix the FutureWarning and DeprecationWarnings sometime this week.

@Theelx Theelx merged commit b71c5de into main Jan 9, 2022
@Theelx Theelx deleted the skip-demjson-test branch January 9, 2022 17:00
@davvid
Copy link
Member

davvid commented Jan 9, 2022

Users that were using demjson may have been relying on its non-standard handling of comments. It's made me wonder whether we should support yaml backends instead. (not related to this cleanup, just thinking out loud)

In a sense, the core problem in jsonpickle could almost be thought of as a dictpickle, and we can support multiple ways to de/serialize from/to that dict.

If users really need comments, relying on a yaml backend sounds like a better long-term solution than a non-standard json parser. Of course, if demjson ever starts getting updated then we can always revisit this decision.

Thanks for cleaning out the demjson backend.

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