FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-132631: Fix json.tool when using --json-lines and a file as input by mpkocher · Pull Request #138961 · python/cpython · GitHub

/ cpython Public

gh-132631: Fix json.tool when using --json-lines and a file as input - #138961

Closed
mpkocher wants to merge 5 commits into
python:mainfrom
mpkocher:fix-json-tool
ConversationCommits (5)ChecksFiles changed
Closed

gh-132631: Fix json.tool when using --json-lines and a file as input#138961
mpkocher wants to merge 5 commits into
python:mainfrom
mpkocher:fix-json-tool

Conversation

mpkocher commented Sep 16, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor
  • Fixes case where --json-lines and a positional file is provided to json.tool
  • Add a specific test for this case
gh-138960: Fix json.tool when using --json-lines

mpkocher commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

This is perhaps the most minimal fix, however, there's an alternative version that would be more invasive.

Some observations:

  • We should try to use standard python constructs if possible. In this case using context managers would make sure the file handle is closed gracefully.
  • We should try to document non-obvious error handling. The ValueError catching isn't particularly obvious (file closing case?). It's also not obvious why main function is doing some error handling/catching, and there's a separate error handling in __main__.
  • Not really a fan of the duplicate for obj in objs copy and pasting. Python has functions as first-class citizens, we should be able to write a single for obj in objs: do_stuff(obj) to encapsulate the optional colorization and translating to json functionality.

mpkocher changed the title Fix json.tool when using --json-lines and a file as input. gh-138960 : Fix json.tool when using --json-lines and a file as input. Sep 16, 2025
bedevere-app Bot mentioned this pull request Sep 16, 2025
Closed
picnixz reviewed Sep 16, 2025
View reviewed changes
Comment thread Lib/json/tool.py Outdated
for obj in objs:
json.dump(obj, outfile, **dump_args)
outfile.write('\n')
if infile is not sys.stdin:

picnixz Sep 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be a finally I think. If there is an exception happening in json.dumps for instance, we would have it left open. So this should be moved after the except ValueError as e with some finally.

mpkocher Sep 17, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I tried looking at the git history of this file and it's a bit tricky to understand why it's not using content managers. At one point it did have context manager approach, then it was migrated to manually calling of open.

If there's a friction point, it's the "in place" feature. I don't think the in place feature/requirement ever fundamentally worked for the json lines case.

picnixz reviewed Sep 16, 2025
View reviewed changes
Comment thread Lib/json/tool.py Outdated
if options.json_lines:
objs = (json.loads(line) for line in infile)
else:
objs = (json.load(infile),)

picnixz Sep 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This one can fail directl, so please be careful that we close the infile beforehand (that's the reaason why we have the try-finally in the first place).

nineteendo Sep 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We're probably using a generator here to save memory. Otherwise we could've just used a list comprehension.

picnixz Sep 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

My comment was about the else branch. It's not a generator, it's a regular tuple.

hugovk commented Sep 16, 2025

Copy link
Copy Markdown
Member

See PR #132632 for another attempt.

hugovk changed the title gh-138960 : Fix json.tool when using --json-lines and a file as input. gh-132631: Fix json.tool when using --json-lines and a file as input Sep 16, 2025
bedevere-app Bot mentioned this pull request Sep 16, 2025
Closed

mpkocher commented Sep 17, 2025

Copy link
Copy Markdown
Contributor Author

The only test that is failing for me locally is the test_writing_in_place.

The "in place" functionality is a bit surprising and doesn't really have clear requirements around error handling.

It's a bit disconcerting that in the "in place" mode you can overwrite/delete the original file if there's an error.

hugovk commented Apr 23, 2026

Copy link
Copy Markdown
Member

Closed in favour of #132632, thanks anyway!

hugovk closed this Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

picnixz picnixz left review comments
+1 more reviewer
nineteendo nineteendo left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL