Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc9ee36
Close file after running the generator
mpkocher Sep 16, 2025
f72edc5
Add test for json lines with an explicit file
mpkocher Sep 16, 2025
2d5e1bf
Use context managers for in and out
mpkocher Sep 16, 2025
ed78614
Merge branch 'main' into fix-json-tool
mpkocher Sep 17, 2025
30a3947
Merge branch 'main' into fix-json-tool
mpkocher Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions Lib/json/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,11 @@ def main():
infile = sys.stdin
else:
infile = open(options.infile, encoding='utf-8')
try:
if options.json_lines:
objs = (json.loads(line) for line in infile)
else:
objs = (json.load(infile),)
finally:
if infile is not sys.stdin:
infile.close()

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.


if options.outfile is None:
outfile = sys.stdout
Expand All @@ -111,6 +108,8 @@ def main():
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.

infile.close()
except ValueError as e:
raise SystemExit(e)

Expand Down
8 changes: 8 additions & 0 deletions Lib/test/test_json/test_tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,14 @@ def test_jsonlines(self):
self.assertEqual(process.stdout, self.jsonlines_expect)
self.assertEqual(process.stderr, '')

@force_not_colorized
def test_jsonlines_with_file(self):
infile = self._create_infile(self.jsonlines_raw)
args = sys.executable, '-m', self.module, '--json-lines', infile
process = subprocess.run(args, capture_output=True, text=True, check=True)
self.assertEqual(process.stdout, self.jsonlines_expect)
self.assertEqual(process.stderr, '')

def test_help_flag(self):
rc, out, err = assert_python_ok('-m', self.module, '-h',
PYTHON_COLORS='0')
Expand Down
Loading