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 unused indent_level from Modules/_json.c #95382

Open
aivarsk opened this issue Jul 28, 2022 · 5 comments
Open

Remove unused indent_level from Modules/_json.c #95382

aivarsk opened this issue Jul 28, 2022 · 5 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@aivarsk
Copy link
Contributor

aivarsk commented Jul 28, 2022

There are commits from 2009-05-02 calculating indent_level but not
using it to indent. There are some other commits from 2011-02-22
commenting out the code with the message "it does not work". Nobody cared
enough about it for 10 years, time to remove commented code and leftovers.

Linked PRs

@corona10
Copy link
Member

Maybe we need to decide not to implement it forever or should be implemented.
I need to understand the historical log for this issue.

@aivarsk
Copy link
Contributor Author

aivarsk commented Aug 4, 2022

If that helps, when indent is requested (json.dumps(obj, indent=4)) it uses the pure Python implementation. C implementation is called when indent is not requested
https://github.com/python/cpython/blob/main/Lib/json/encoder.py#L247

@aivarsk
Copy link
Contributor Author

aivarsk commented Oct 22, 2022

@corona10 Any updates on this? The proposal is to remove some dead and incomplete code from the json encoder C implementation that is either commented-out or only increments the indent (all decrements are commented-out) and eliminate an unused function parameter. Right now it uses the Python implementation when an indentation is requested.

I can also update the C encoder to do the indentation correctly if that is the way to proceed.

@corona10
Copy link
Member

I can also update the C encoder to do the indentation correctly if that is the way to proceed.

I think that this way the right way to do, would you like to update the C implementation with a benchmark?

Thanks

@eendebakpt
Copy link
Contributor

@corona10 @aivarsk The relatively slow json encoding for indent!=None was a small bottleneck in our code, so I decided to implement it (instead of removing the old dead code). Feedback on the implementation is welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
Status: No status
Development

No branches or pull requests

4 participants