Skip to content

Conversation

@valentinoli
Copy link
Contributor

@valentinoli valentinoli commented Jul 18, 2025

If result.stdout contains something more than the JSON output of all the environments, such as logs from Hatch plugins/hooks, this will result in JSONDecodeError. We must extract the relevant output to prevent such cases.

If `result.stdout` contains something more than the JSON output of all the environments, such as logs from Hatch plugins/hooks, this will result in JSONDecodeError. We must extract the relevant output to prevent such cases.
@juftin juftin requested a review from Copilot July 19, 2025 03:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where parsing the output of hatch env show --json could fail with a JSONDecodeError when the stdout contains additional content beyond the JSON output, such as logs from Hatch plugins or hooks. The fix extracts only the relevant JSON output by taking the last line of stdout.

  • Adds robust JSON output extraction from hatch env show --json command
  • Prevents JSONDecodeError when stdout contains plugin/hook logs before the JSON output
Comments suppressed due to low confidence (1)

hatch_pip_compile/cli.py:146

  • [nitpick] The variable name 'envs_raw' is somewhat ambiguous. Consider renaming it to 'json_output' or 'last_line_output' to better reflect that it contains the extracted JSON data from the last line.
        envs_raw = result.stdout.strip().rsplit(b"\n", maxsplit=1)[-1]

@juftin
Copy link
Owner

juftin commented Jul 19, 2025

lol, that was my first time requesting review from co-pilot. It wasn't actually that useful.

Thanks for the contribution, @valentinoli - give me a day or to get a closer look at this but everything looks good on first glance.

@valentinoli
Copy link
Contributor Author

Perhaps consider parsing hatch.toml/pyproject.toml directly instead

@valentinoli
Copy link
Contributor Author

@juftin

Consider #96 as an alternative

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants