Remove `simplejson` in favor of using `json` #7035

christian-monch · 2022-09-14T11:18:25Z

Fixes issue #7034

This PR removes simplejson in favor of the "builtin" json-module.

Remark: this changes are also contained in PR #7014

In addition this PR fixes the test datalad.cli.tests.test_main.test_help_np, by adapting the group names used in the test to the group names of the command groups.

Changelog

🪓 Deprecations and removals

replace simplejson with json and remove simplejson

This commit removes `simplejson` dependency in favor of the built-in `json`-module. Encoding is now a property of the streams and not the encoder.

This commit removes a call to `json_loads()`, which has been removed during deprecation and moving of core-metadata code to `datalad-deprecated`.

This commit fixes the test that verifies the command group names. It basically adapts the verified command group names to the changes from 8decbdc

codecov · 2022-09-14T19:37:58Z

Codecov Report

Base: 75.23% // Head: 76.00% // Increases project coverage by +0.76% 🎉

Coverage data is based on head (5f247b9) compared to base (5fa362e).
Patch coverage: 71.42% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7035      +/-   ##
==========================================
+ Coverage   75.23%   76.00%   +0.76%     
==========================================
  Files         357      357              
  Lines       59213    59291      +78     
  Branches     6615     6615              
==========================================
+ Hits        44551    45063     +512     
+ Misses      14646    14212     -434     
  Partials       16       16

Impacted Files	Coverage Δ
datalad/support/annexrepo.py	`75.35% <42.85%> (-0.15%)`	⬇️
datalad/support/json_py.py	`88.97% <100.00%> (-11.03%)`	⬇️
datalad/support/tests/test_json_py.py	`74.07% <100.00%> (-0.93%)`	⬇️
datalad/_version.py	`45.68% <0.00%> (-0.28%)`	⬇️
datalad/tests/utils.py	`56.58% <0.00%> (+7.92%)`	⬆️
datalad/tests/test_tests_utils.py	`68.55% <0.00%> (+68.55%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

yarikoptic · 2022-09-14T20:10:49Z

Is there comment in commit where we started to use simplejson - was there some specific reason or it was so long ago that there were no built in json?

christian-monch · 2022-09-15T06:55:31Z

Is there comment in commit where we started to use simplejson - was there some specific reason or it was so long ago that there were no built in json?

Yes, there are two:

The first

commit 5513caae92e82d26f0e90b1e642d18f9cb43e0a1
Author: Michael Hanke <michael.hanke@gmail.com>
Date:   Tue Aug 9 20:57:39 2016 +0200

    RF: Switch to simplejson, with default handling of utf-8
    
    Doesn't require additional jumps like the stdlib json module

I think at least since python 3.6, utf-8 encoded JSON should be properly handled by the built-in json-module

The second

In datalad/support/annexrepo.py:57 is a comment that

# must not be loads, because this one would log, and we need to log ourselves
from datalad.support.json_py import json_loads

I am not sure if the comment is still true/valid.

yarikoptic · 2022-09-15T14:44:13Z

datalad/support/json_py.py

-# simply mirrored for now
-from simplejson import loads as json_loads
-from simplejson import JSONDecodeError
+import json


To minimize impact and possibly avoid some part of this PR diff I would have:

Suggested change

import json

import json

from json import JSONDecodeError

This would not be sufficient. There would need to be replacements for jsonload, jsondump, json_loads too.

I personally think a clean cut is better than yet another intermediate import target to preserve forever.

mih

LGTM! Thx!

My memory also confirms that the reason for simplejson was UTF handling with PY2. No longer a necessity.

mih · 2022-09-16T08:14:19Z

datalad/support/json_py.py

-# simply mirrored for now
-from simplejson import loads as json_loads
-from simplejson import JSONDecodeError
+import json


This would not be sufficient. There would need to be replacements for jsonload, jsondump, json_loads too.

I personally think a clean cut is better than yet another intermediate import target to preserve forever.

codeclimate · 2022-09-16T10:49:25Z

Code Climate has analyzed commit 5f247b9 and detected 0 issues on this pull request.

View more on Code Climate.

yarikoptic-gitmate · 2022-12-31T20:17:42Z

PR released in 0.18.0

christian-monch added 2 commits September 14, 2022 12:22

remove simplejson dependency

98e9a9f

This commit removes `simplejson` dependency in favor of the built-in `json`-module. Encoding is now a property of the streams and not the encoder.

remove call to removed json_loads-method

93401a2

This commit removes a call to `json_loads()`, which has been removed during deprecation and moving of core-metadata code to `datalad-deprecated`.

christian-monch added the semver-internal Changes only affect the internal API label Sep 14, 2022

christian-monch requested a review from mih September 14, 2022 11:21

christian-monch mentioned this pull request Sep 14, 2022

Remove metadata-related code after its move to -deprecated #7014

Merged

7 tasks

adapt tests to modified command group names

aee2cf1

This commit fixes the test that verifies the command group names. It basically adapts the verified command group names to the changes from 8decbdc

yarikoptic reviewed Sep 15, 2022

View reviewed changes

mih approved these changes Sep 16, 2022

View reviewed changes

Merge branch 'master' into issue-7034-remove-simplejson

5f247b9

christian-monch merged commit b1d7251 into datalad:master Sep 16, 2022

christian-monch deleted the issue-7034-remove-simplejson branch September 16, 2022 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `simplejson` in favor of using `json` #7035

Remove `simplejson` in favor of using `json` #7035

christian-monch commented Sep 14, 2022 •

edited

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

yarikoptic commented Sep 14, 2022

christian-monch commented Sep 15, 2022 •

edited

Loading

yarikoptic Sep 15, 2022

mih Sep 16, 2022

mih left a comment •

edited

Loading

mih Sep 16, 2022

codeclimate bot commented Sep 16, 2022

yarikoptic-gitmate commented Dec 31, 2022

Remove simplejson in favor of using json #7035

Remove simplejson in favor of using json #7035

Conversation

christian-monch commented Sep 14, 2022 • edited Loading

Changelog

🪓 Deprecations and removals

codecov bot commented Sep 14, 2022 • edited Loading

Codecov Report

yarikoptic commented Sep 14, 2022

christian-monch commented Sep 15, 2022 • edited Loading

The first

The second

yarikoptic Sep 15, 2022

Choose a reason for hiding this comment

mih Sep 16, 2022

Choose a reason for hiding this comment

mih left a comment • edited Loading

Choose a reason for hiding this comment

mih Sep 16, 2022

Choose a reason for hiding this comment

codeclimate bot commented Sep 16, 2022

yarikoptic-gitmate commented Dec 31, 2022

Remove `simplejson` in favor of using `json` #7035

Remove `simplejson` in favor of using `json` #7035

christian-monch commented Sep 14, 2022 •

edited

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

christian-monch commented Sep 15, 2022 •

edited

Loading

mih left a comment •

edited

Loading