Skip to content

Conversation

@mpkocher
Copy link
Contributor

@mpkocher mpkocher commented Sep 16, 2025

  • 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
Copy link
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 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
Lib/json/tool.py Outdated
for obj in objs:
json.dump(obj, outfile, **dump_args)
outfile.write('\n')
if infile is not sys.stdin:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Lib/json/tool.py Outdated
if options.json_lines:
objs = (json.loads(line) for line in infile)
else:
objs = (json.load(infile),)
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@hugovk
Copy link
Member

hugovk commented Sep 16, 2025

See PR #132632 for another attempt.

@hugovk 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
@mpkocher
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants