feat: add CLI support for json2xml-py #266
Merged
Conversation
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
Contributor
Reviewer's GuideAdds a full-featured Python CLI for json2xml with parity to the Go implementation, exposes it as a console script, and introduces benchmark utilities and CLI test coverage. Sequence diagram for json2xml-py CLI executionsequenceDiagram
actor User
participant Shell
participant json2xml_cli as json2xml_cli_main
participant Argparse
participant Utils as json2xml_utils
participant Converter as Json2xml
participant FS as FileSystem
User->>Shell: json2xml-py [flags] [input]
Shell->>json2xml_cli: main(argv)
json2xml_cli->>Argparse: create_parser()
Argparse-->>json2xml_cli: args
json2xml_cli->>json2xml_cli: read_input(args)
alt URL input
json2xml_cli->>Utils: readfromurl(args.url)
Utils-->>json2xml_cli: data
else String input
json2xml_cli->>Utils: readfromstring(args.string)
Utils-->>json2xml_cli: data
else File input
alt stdin marker
json2xml_cli->>json2xml_cli: read_from_stdin()
json2xml_cli->>Utils: readfromstring(stdin_data)
Utils-->>json2xml_cli: data
else path to file
json2xml_cli->>Utils: readfromjson(args.input_file)
Utils-->>json2xml_cli: data
end
else Stdin with no args
json2xml_cli->>json2xml_cli: read_from_stdin()
json2xml_cli->>Utils: readfromstring(stdin_data)
Utils-->>json2xml_cli: data
end
json2xml_cli->>Converter: Json2xml(data, wrapper, root, pretty, attr_type, item_wrap, xpath_format)
Converter-->>json2xml_cli: xml_output
json2xml_cli->>FS: write_output(xml_output, args.output)
FS-->>User: XML to file or stdout
Flow diagram for benchmark.py Python vs Go CLI comparisonflowchart TD
A[Start benchmark.py] --> B[Check Go binary and paths]
B --> C[Generate random large and very large JSON files]
C --> D[Load small inline JSON and medium example file]
D --> E[For each dataset size]
E --> F[Run Python CLI iterations via subprocess]
F --> G[Collect timing stats for Python]
E --> H[Run Go CLI iterations via subprocess]
H --> I[Collect timing stats for Go]
G --> J[Aggregate results]
I --> J
J --> K[Compute averages and relative speedups]
K --> L[Print colored per-size summary]
L --> M[Print overall comparison and exit]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
vinitkumar
force-pushed
the
feat/cli-tool
branch
from
90ea173 to
b1f79c4
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 security issues, 10 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The CLI defines and documents
-c/--cdataand-l/--list-headersflags but never passes them intoJson2xml, so these options currently have no effect; consider wiring them through to the converter or dropping the flags. - Both
benchmark.pyandbenchmark.shcontain hard-coded absolute paths to your local filesystem, which will break for other users and CI; using paths relative to the project root or environment variables would make these scripts portable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CLI defines and documents `-c/--cdata` and `-l/--list-headers` flags but never passes them into `Json2xml`, so these options currently have no effect; consider wiring them through to the converter or dropping the flags.
- Both `benchmark.py` and `benchmark.sh` contain hard-coded absolute paths to your local filesystem, which will break for other users and CI; using paths relative to the project root or environment variables would make these scripts portable.
## Individual Comments
### Comment 1
<location> `json2xml/cli.py:345-354` </location>
<code_context>
+
+ # Convert to XML
+ try:
+ converter = Json2xml(
+ data=data,
+ wrapper=args.wrapper,
+ root=args.root,
+ pretty=args.pretty,
+ attr_type=args.attr_type,
+ item_wrap=args.item_wrap,
+ xpath_format=args.xpath_format,
+ )
+ xml_output = converter.to_xml()
+
+ if xml_output is None:
</code_context>
<issue_to_address>
**issue (bug_risk):** CLI options `--cdata` and `--list-headers` are parsed but never passed into Json2xml, so they currently have no effect.
`args.cdata` and `args.list_headers` are parsed but never passed to `Json2xml`, so `--cdata` and `--list-headers` are effectively no-ops. Please either forward these flags to the constructor (e.g. `cdata=args.cdata`, `list_headers=args.list_headers`) if supported, or remove them from the CLI to avoid a misleading option surface.
</issue_to_address>
### Comment 2
<location> `benchmark.py:12-24` </location>
<code_context>
+
+
+# Paths
+PYTHON_CLI = [sys.executable, "-m", "json2xml.cli"]
+GO_CLI = Path("/Users/vinitkumar/projects/go/json2xml-go/json2xml-go")
+EXAMPLES_DIR = Path("/Users/vinitkumar/projects/python/json2xml/examples")
+
+# Colors for terminal output
</code_context>
<issue_to_address>
**suggestion:** Hard-coded absolute paths in the benchmark reduce portability and make it harder for others to run.
`GO_CLI` and `EXAMPLES_DIR` are set to user-specific absolute paths under `/Users/vinitkumar/...`, so the benchmark only works in your local setup. Please make these configurable (e.g., via environment variables, CLI args, or paths relative to `__file__`/repo root) so others can run the benchmarks without modifying the script.
```suggestion
import os
import random
import string
import subprocess
import sys
import tempfile
import time
from pathlib import Path
# Paths
PYTHON_CLI = [sys.executable, "-m", "json2xml.cli"]
# Base directory for repo-relative defaults
BASE_DIR = Path(__file__).resolve().parent
DEFAULT_EXAMPLES_DIR = BASE_DIR.parent / "examples"
# Allow overriding via environment variables; otherwise use sensible defaults
# - JSON2XML_GO_CLI: path to the json2xml-go binary (can be absolute or relative)
# - JSON2XML_EXAMPLES_DIR: path to the examples directory
GO_CLI = Path(os.environ.get("JSON2XML_GO_CLI", "json2xml-go"))
EXAMPLES_DIR = Path(os.environ.get("JSON2XML_EXAMPLES_DIR", str(DEFAULT_EXAMPLES_DIR)))
```
</issue_to_address>
### Comment 3
<location> `benchmark.sh:23-26` </location>
<code_context>
+
+# Configuration
+PYTHON_CLI="python -m json2xml.cli"
+GO_CLI="/Users/vinitkumar/projects/go/json2xml-go/json2xml-go"
+ITERATIONS=10
+SMALL_JSON='{"name": "John", "age": 30, "city": "New York"}'
+MEDIUM_JSON="$(cat /Users/vinitkumar/projects/python/json2xml/examples/bigexample.json)"
+LARGE_JSON_FILE="/tmp/json2xml_benchmark_large.json"
+
</code_context>
<issue_to_address>
**suggestion:** The shell benchmark script also relies on user-specific absolute paths, limiting reuse across environments.
`GO_CLI` and `MEDIUM_JSON` are hardcoded to `/Users/vinitkumar/...`, so the script breaks if run from another checkout or in CI. Consider resolving the repo path relative to the script (e.g. via `SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)`) or passing these paths via arguments/env vars with sensible defaults.
Suggested implementation:
```
# Configuration
PYTHON_CLI="${PYTHON_CLI:-python -m json2xml.cli}"
# Resolve repository-relative paths based on this script's location
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
# Go CLI: can be overridden via GO_CLI env var; default is relative to repo layout
GO_CLI="${GO_CLI:-"$SCRIPT_DIR/../../go/json2xml-go/json2xml-go"}"
# Medium JSON: can be overridden via MEDIUM_JSON_FILE env var; default is examples/bigexample.json
MEDIUM_JSON_FILE="${MEDIUM_JSON_FILE:-"$SCRIPT_DIR/examples/bigexample.json"}"
MEDIUM_JSON="$(cat "$MEDIUM_JSON_FILE")"
set -e
```
If your repo layout differs from the assumed structure:
1. Update the relative path in `GO_CLI` to point from `benchmark.sh` to the `json2xml-go` binary.
2. If `bigexample.json` is not under `examples/` relative to `benchmark.sh`, adjust `MEDIUM_JSON_FILE`'s default path accordingly.
You may also want to document the `GO_CLI` and `MEDIUM_JSON_FILE` env vars in your README or script usage comments.
</issue_to_address>
### Comment 4
<location> `tests/test_cli.py:41-50` </location>
<code_context>
+ assert "json2xml-py version" in result.stdout
+ assert "Vinit Kumar" in result.stdout
+
+ def test_string_input(self) -> None:
+ """Test -s/--string flag for inline JSON."""
+ result = subprocess.run(
+ [sys.executable, "-m", "json2xml.cli", "-s", '{"name": "John"}'],
+ capture_output=True,
+ text=True,
+ )
+ assert result.returncode == 0
+ assert "<name" in result.stdout
+ assert "John" in result.stdout
+
+ def test_file_input(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for URL input, including error handling
The `-u/--url` option and `URLReadError` branch in `read_input` aren’t covered by tests, so both success and failure for URL input are unverified. Please add tests that:
1) Cover successful URL input by monkeypatching `json2xml.cli.readfromurl` to return a known dict and asserting the resulting XML.
2) Cover the error path by monkeypatching `readfromurl` to raise `URLReadError` and asserting exit code 1 and the expected `stderr` message.
You can simplify this by importing `json2xml.cli` and calling `read_input()`/`main()` directly rather than using subprocess for these cases.
Suggested implementation:
```python
import subprocess
import pytest
```
```python
assert "John" in result.stdout
def test_file_input(self) -> None:
"""Test file input."""
with tempfile.NamedTemporaryFile(
mode="w", suffix=".json", delete=False
) as f:
f.write('{"key": "value"}')
f.flush()
result = subprocess.run(
[sys.executable, "-m", "json2xml.cli", f.name],
capture_output=True,
text=True,
)
assert result.returncode == 0
assert "<key" in result.stdout
assert "value" in result.stdout
def test_url_input_success(self, monkeypatch, capsys) -> None:
"""Test -u/--url flag with successful URL read."""
import json2xml.cli as cli
def fake_readfromurl(url: str) -> dict:
assert url == "http://example.com/data.json"
return {"user": {"name": "Alice"}}
monkeypatch.setattr(cli, "readfromurl", fake_readfromurl)
# Call CLI entry point directly instead of spawning a subprocess
with pytest.raises(SystemExit) as excinfo:
cli.main(["-u", "http://example.com/data.json"])
assert excinfo.value.code == 0
captured = capsys.readouterr()
# Basic sanity checks on the generated XML
assert "<user" in captured.out
assert "<name" in captured.out
assert "Alice" in captured.out
def test_url_input_error(self, monkeypatch, capsys) -> None:
"""Test -u/--url flag when URLReadError is raised."""
import json2xml.cli as cli
def fake_readfromurl(url: str) -> dict:
raise cli.URLReadError("Failed to read URL")
monkeypatch.setattr(cli, "readfromurl", fake_readfromurl)
with pytest.raises(SystemExit) as excinfo:
cli.main(["-u", "http://example.com/data.json"])
assert excinfo.value.code == 1
captured = capsys.readouterr()
# Ensure the error message from URLReadError is propagated to stderr
assert "Failed to read URL" in captured.err
```
These tests assume:
1. `json2xml.cli.main` accepts a list of arguments (e.g. `main(["-u", "..."])`) and exits via `SystemExit(code)`. If `main` instead reads `sys.argv` or has a different signature, adjust the call accordingly or wrap it in a helper.
2. `URLReadError` is exposed as `json2xml.cli.URLReadError` and `readfromurl` is a top-level callable in `json2xml.cli`. If they live in a different module (e.g. `json2xml.utils`), update the imports and `monkeypatch.setattr` targets to match the actual locations.
</issue_to_address>
### Comment 5
<location> `tests/test_cli.py:141-150` </location>
<code_context>
+ assert len(lines) <= 2
+ assert "<name" in result.stdout
+
+ def test_no_type_option(self) -> None:
+ """Test --no-type flag excludes type attributes."""
+ result = subprocess.run(
+ [
+ sys.executable, "-m", "json2xml.cli",
+ "-s", '{"name": "John", "age": 30}',
+ "--no-type",
+ ],
+ capture_output=True,
+ text=True,
+ )
+ assert result.returncode == 0
+ assert 'type="str"' not in result.stdout
+ assert 'type="int"' not in result.stdout
+
+ def test_xpath_format(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Cover remaining conversion flags: item-wrap, cdata, and list-headers
Current tests only cover `--no-root`, `--no-pretty`, `--no-type`, and `-x/--xpath`. Please add cases for:
- `-i/--item-wrap` and `--no-item-wrap`
- `-c/--cdata`
- `-l/--list-headers`
so that we assert on the resulting XML structure (e.g., `<item>` wrappers, CDATA sections, repeated headers) and verify these flags are correctly wired through to `Json2xml`.
Suggested implementation:
```python
def test_xpath_format(self) -> None:
"""Test -x/--xpath flag for XPath 3.1 format."""
result = subprocess.run(
[
sys.executable,
"-m",
"json2xml.cli",
"-s",
'{"name": "John"}',
"-x",
],
capture_output=True,
text=True,
)
assert result.returncode == 0
# XPath 3.1 serialization should use map{} syntax
assert "map{" in result.stdout or "map {" in result.stdout
def test_item_wrap_option(self) -> None:
"""Test -i/--item-wrap flag adds <item> wrappers for list elements."""
result = subprocess.run(
[
sys.executable,
"-m",
"json2xml.cli",
"-s",
'{"numbers": [1, 2]}',
"-i",
],
capture_output=True,
text=True,
)
assert result.returncode == 0
# When item-wrap is enabled, list elements should be wrapped in <item> tags
assert "<item" in result.stdout
def test_no_item_wrap_option(self) -> None:
"""Test --no-item-wrap flag disables <item> wrappers for list elements."""
result = subprocess.run(
[
sys.executable,
"-m",
"json2xml.cli",
"-s",
'{"numbers": [1, 2]}',
"--no-item-wrap",
],
capture_output=True,
text=True,
)
assert result.returncode == 0
# When item-wrap is disabled, there should be no <item> tag wrappers
assert "<item" not in result.stdout
def test_cdata_option(self) -> None:
"""Test -c/--cdata flag wraps string data in CDATA sections."""
result = subprocess.run(
[
sys.executable,
"-m",
"json2xml.cli",
"-s",
'{"text": "<tag> & stuff"}',
"-c",
],
capture_output=True,
text=True,
)
assert result.returncode == 0
# CDATA should be used instead of escaping special characters
assert "<![CDATA[" in result.stdout
assert "text" in result.stdout
def test_list_headers_option(self) -> None:
"""Test -l/--list-headers flag affects list-of-maps serialization."""
json_input = '[{"name": "John", "age": 30}, {"name": "Jane", "age": 25}]'
result = subprocess.run(
[
sys.executable,
"-m",
"json2xml.cli",
"-s",
json_input,
"-l",
],
capture_output=True,
text=True,
)
assert result.returncode == 0
# With list-headers enabled, header information should be present in the XML.
# Depending on implementation this might introduce header elements or attributes;
# at minimum we expect to see the keys represented in a header-like structure.
# These assertions intentionally remain structural rather than exact-string.
assert "name" in result.stdout
assert "age" in result.stdout
```
If the actual XPath, item-wrap, CDATA, or list-headers output formats differ from these assumptions, adjust the assertions to match the concrete XML produced by your `Json2xml` implementation. For example:
- For `test_xpath_format`, assert on the precise XPath 3.1 string that `Json2xml` emits.
- For `test_item_wrap_option` / `test_no_item_wrap_option`, you can parse `result.stdout` with an XML parser and assert on the presence/absence of `<item>` elements under the list parent.
- For `test_cdata_option`, assert that the element value is inside a CDATA node rather than a text node with escaped characters.
- For `test_list_headers_option`, replace the generic `"name"` / `"age"` checks with assertions targeting the exact header elements/attributes your library produces when `list_headers` is enabled.
</issue_to_address>
### Comment 6
<location> `tests/test_cli.py:52-61` </location>
<code_context>
+ assert "<name" in result.stdout
+ assert "John" in result.stdout
+
+ def test_file_input(self) -> None:
+ """Test file input."""
+ with tempfile.NamedTemporaryFile(
+ mode="w", suffix=".json", delete=False
+ ) as f:
+ f.write('{"key": "value"}')
+ f.flush()
+
+ result = subprocess.run(
+ [sys.executable, "-m", "json2xml.cli", f.name],
+ capture_output=True,
+ text=True,
+ )
+
+ Path(f.name).unlink()
+
+ assert result.returncode == 0
+ assert "<key" in result.stdout
+ assert "value" in result.stdout
+
+ def test_output_file(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for file read failures (e.g., nonexistent or unreadable files)
The error path in `read_input` when `readfromjson` raises `JSONReadError` (e.g., nonexistent or invalid JSON file) isn’t exercised. Please add tests that:
- Run the CLI with a non-existent JSON file and assert exit code 1 and an appropriate error on `stderr`.
- Optionally cover an unreadable file (permissions issue) if it’s practical in this test environment.
This will verify the CLI’s error handling for file input failures.
Suggested implementation:
```python
from pathlib import Path
import os
import pytest
```
```python
def test_nonexistent_file(self) -> None:
"""Test behavior when JSON file does not exist."""
with tempfile.TemporaryDirectory() as tmpdir:
missing_file = Path(tmpdir) / "does_not_exist.json"
result = subprocess.run(
[sys.executable, "-m", "json2xml.cli", str(missing_file)],
capture_output=True,
text=True,
)
assert result.returncode == 1
assert result.stderr # should contain an error message
def test_unreadable_file(self) -> None:
"""Test behavior when JSON file is not readable."""
if os.name == "nt":
pytest.skip("Unreadable file test not reliable on Windows")
with tempfile.TemporaryDirectory() as tmpdir:
unreadable_file = Path(tmpdir) / "unreadable.json"
unreadable_file.write_text('{"foo": "bar"}')
# Remove read permissions for everyone
unreadable_file.chmod(0)
result = subprocess.run(
[sys.executable, "-m", "json2xml.cli", str(unreadable_file)],
capture_output=True,
text=True,
)
# Restore permissions so the file can be deleted during cleanup
unreadable_file.chmod(0o600)
assert result.returncode == 1
assert result.stderr # should contain an error message
def test_output_file(self) -> None:
"""Test -o/--output flag for file output."""
with tempfile.TemporaryDirectory() as tmpdir:
output_file = Path(tmpdir) / "output.xml"
result = subprocess.run(
[
sys.executable, "-m", "json2xml.cli",
"-s", '{"test": "data"}',
"-o", str(output_file),
],
```
</issue_to_address>
### Comment 7
<location> `tests/test_cli.py:170-179` </location>
<code_context>
+ assert result.returncode == 0
+ assert "http://www.w3.org/2005/xpath-functions" in result.stdout
+
+ def test_stdin_input(self) -> None:
+ """Test reading from stdin with - argument."""
+ result = subprocess.run(
+ [sys.executable, "-m", "json2xml.cli", "-"],
+ input='{"stdin": "test"}',
+ capture_output=True,
+ text=True,
+ )
+ assert result.returncode == 0
+ assert "<stdin" in result.stdout
+ assert "test" in result.stdout
+
+ def test_invalid_json_string(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Test stdin error paths, including empty stdin with `-` and non-tty stdin
Two error paths are still untested:
1) `read_from_stdin` with empty input (should print `"Error: Empty input"` and exit with code 1).
2) `read_input` when stdin is non-tty and empty (falls through to the same empty-input logic).
Please add tests that call `json2xml.cli -` with `input=""` and that simulate a non-tty stdin with no data to cover these cases and lock in the expected error message and exit code.
</issue_to_address>
### Comment 8
<location> `tests/test_cli.py:182-190` </location>
<code_context>
+ assert "<stdin" in result.stdout
+ assert "test" in result.stdout
+
+ def test_invalid_json_string(self) -> None:
+ """Test error handling for invalid JSON string."""
+ result = subprocess.run(
+ [sys.executable, "-m", "json2xml.cli", "-s", "not valid json"],
+ capture_output=True,
+ text=True,
+ )
+ assert result.returncode == 1
+ assert "Error" in result.stderr
+
+ def test_no_input_error(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen negative tests by asserting on specific error messages
The invalid JSON test only checks for exit code 1 and a generic "Error" in stderr, which could still pass if a different code path prints the same word. Please assert on a more specific substring (e.g., "Error parsing JSON string" from `read_input`/`readfromstring`) so the test validates the correct error path.
```suggestion
def test_invalid_json_string(self) -> None:
"""Test error handling for invalid JSON string."""
result = subprocess.run(
[sys.executable, "-m", "json2xml.cli", "-s", "not valid json"],
capture_output=True,
text=True,
)
assert result.returncode == 1
assert "Error parsing JSON string" in result.stderr
```
</issue_to_address>
### Comment 9
<location> `tests/test_cli.py:192-189` </location>
<code_context>
+ assert result.returncode == 1
+ assert "Error" in result.stderr
+
+ def test_no_input_error(self) -> None:
+ """Test error when no input is provided."""
+ # Force isatty to return True by using a pty-like environment
+ # For now, just test with explicit empty args
+ result = subprocess.run(
+ [sys.executable, "-m", "json2xml.cli"],
+ capture_output=True,
+ text=True,
+ input="", # Empty stdin
+ )
+ # Should fail with no input error
+ assert result.returncode == 1
+
+ def test_list_input(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Extend no-input test to assert on stderr and `sys.stdin.isatty` behavior
This test currently only checks the exit code. Please also assert that `stderr` includes the exact message `"Error: No input provided. Use -h for help."`. In addition, consider an in-process test (importing `json2xml.cli` and monkeypatching `sys.stdin.isatty` to return `True`) so the `isatty` branch is covered deterministically rather than depending on the environment.
Suggested implementation:
```python
def test_no_input_error(self) -> None:
"""Test error when no input is provided."""
# For now, test with explicit empty stdin
result = subprocess.run(
[sys.executable, "-m", "json2xml.cli"],
capture_output=True,
text=True,
input="", # Empty stdin
)
# Should fail with no input error
assert result.returncode == 1
assert "Error: No input provided. Use -h for help." in result.stderr
def test_no_input_error_isatty(self, monkeypatch, capsys) -> None:
"""Test no-input error when stdin is a TTY (isatty() is True)."""
import io
from json2xml import cli as cli_mod
class TtyStringIO(io.StringIO):
def isatty(self) -> bool: # type: ignore[override]
return True
# Simulate an interactive TTY with no input
monkeypatch.setattr("sys.stdin", TtyStringIO(""))
# main() should call sys.exit(1) on no input
with pytest.raises(SystemExit) as excinfo:
cli_mod.main()
assert excinfo.value.code == 1
captured = capsys.readouterr()
assert "Error: No input provided. Use -h for help." in captured.err
def test_list_input(self) -> None:
```
To make this compile and run:
1. Ensure `pytest` is imported at the top of `tests/test_cli.py` (e.g., `import pytest`) if it is not already.
2. If `io` is preferred to be imported at module level instead of inside the test, move `import io` from inside `test_no_input_error_isatty` to the import section at the top.
3. If the CLI entry-point function is not named `main`, adjust `cli_mod.main()` to the correct function used by `json2xml.cli` when invoked as a module (`python -m json2xml.cli`).
</issue_to_address>
### Comment 10
<location> `tests/test_cli.py:218-227` </location>
<code_context>
+ assert result.returncode == 0
+ assert "<item" in result.stdout
+
+ def test_nested_json(self) -> None:
+ """Test handling of nested JSON structures."""
+ nested_json = '{"outer": {"inner": {"value": 42}}}'
+ result = subprocess.run(
+ [sys.executable, "-m", "json2xml.cli", "-s", nested_json],
+ capture_output=True,
+ text=True,
+ )
+ assert result.returncode == 0
+ assert "<outer" in result.stdout
+ assert "<inner" in result.stdout
+ assert "42" in result.stdout
</code_context>
<issue_to_address>
**suggestion (testing):** Consider unit-level tests for conversion error branches (to_xml returns None or raises)
Current tests only cover successful CLI runs via subprocess. The `main` function’s two error paths are untested:
- `Json2xml.to_xml()` returns `None` → prints `"Error: Empty data, no XML generated"` and exits 1.
- `Json2xml` construction or `to_xml()` raises → prints `"Error converting to XML: ..."` and exits 1.
To cover these, add unit tests that import `json2xml.cli.main` and monkeypatch `json2xml.cli.Json2xml` so that it:
- Returns an object whose `to_xml()` returns `None`.
- Raises a custom exception from `to_xml()`.
Then assert on the exit code and `stderr` output to verify both error branches.
Suggested implementation:
```python
assert result.returncode == 0
assert "<outer" in result.stdout
assert "<inner" in result.stdout
assert "42" in result.stdout
def test_main_to_xml_returns_none(self, monkeypatch, capsys) -> None:
"""Json2xml.to_xml() returns None -> main() exits 1 with empty-data error."""
import json2xml.cli as cli
import pytest
class DummyJson2xml:
def __init__(self, *_, **__):
pass
def to_xml(self):
return None
monkeypatch.setattr(cli, "Json2xml", DummyJson2xml)
with pytest.raises(SystemExit) as excinfo:
cli.main(["-s", '{"key": "value"}'])
assert excinfo.value.code == 1
captured = capsys.readouterr()
assert "Error: Empty data, no XML generated" in captured.err
def test_main_to_xml_raises_exception(self, monkeypatch, capsys) -> None:
"""Json2xml.to_xml() raises -> main() exits 1 with conversion error."""
import json2xml.cli as cli
import pytest
class FailingJson2xml:
def __init__(self, *_, **__):
pass
def to_xml(self):
raise RuntimeError("boom")
monkeypatch.setattr(cli, "Json2xml", FailingJson2xml)
with pytest.raises(SystemExit) as excinfo:
cli.main(["-s", '{"key": "value"}'])
assert excinfo.value.code == 1
captured = capsys.readouterr()
assert "Error converting to XML: boom" in captured.err
```
If the test class containing these methods is not already using `monkeypatch` and `capsys` fixtures, ensure that `pytest` is being used as the test runner so those fixtures are available. If you prefer module-level imports, you can move the `import pytest` and `import json2xml.cli as cli` statements to the top of `tests/test_cli.py` instead of importing inside the test functions.
</issue_to_address>
### Comment 11
<location> `benchmark.py:87` </location>
<code_context>
subprocess.run(cmd, capture_output=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 12
<location> `benchmark.py:92` </location>
<code_context>
result = subprocess.run(cmd, capture_output=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+170
to
+175
| def test_stdin_input(self) -> None: | ||
| """Test reading from stdin with - argument.""" | ||
| result = subprocess.run( | ||
| [sys.executable, "-m", "json2xml.cli", "-"], | ||
| input='{"stdin": "test"}', | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| assert result.returncode == 0 | ||
| assert "<stdin" in result.stdout |
Contributor
There was a problem hiding this comment.
suggestion (testing): Test stdin error paths, including empty stdin with - and non-tty stdin
Two error paths are still untested:
read_from_stdinwith empty input (should print"Error: Empty input"and exit with code 1).read_inputwhen stdin is non-tty and empty (falls through to the same empty-input logic).
Please add tests that call json2xml.cli - with input="" and that simulate a non-tty stdin with no data to cover these cases and lock in the expected error message and exit code.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #266 +/- ##
===========================================
- Coverage 99.69% 76.79% -22.90%
===========================================
Files 3 4 +1
Lines 330 431 +101
===========================================
+ Hits 329 331 +2
- Misses 1 100 +99
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
vinitkumar
force-pushed
the
feat/cli-tool
branch
from
b1f79c4 to
6617e76
Compare
- Add json2xml/cli.py with same flags as Go version
- Add console script entry point as json2xml-py
- Add comprehensive CLI tests (15 tests)
- Add benchmark scripts to compare Python vs Go performance
Flags: -w/--wrapper, -r/--root, -p/--pretty, -t/--type,
-i/--item-wrap, -x/--xpath, -c/--cdata, -l/--list-headers,
-u/--url, -s/--string, -o/--output, -v/--version, -h/--help
vinitkumar
force-pushed
the
feat/cli-tool
branch
from
6617e76 to
88b12ae
Compare
- Add cdata and list_headers parameters to Json2xml class (bug fix) - Pass --cdata and --list-headers CLI flags to Json2xml converter - Make benchmark paths configurable via environment variables - Remove unused imports (os, pytest, CaptureFixture) - Add tests for cdata, list_headers, and stdin error paths - Total tests: 210 (19 CLI tests)
Compare performance across: - CPython 3.14.2 (baseline) - CPython 3.15.0a4 (1.16x faster) - PyPy 3.10.16 (1.22x faster) - Go json2xml-go (7.34x faster) Key findings: - Go is 7-20x faster than Python implementations - CPython 3.15 shows 13-35% improvement over 3.14 - PyPy excels at large inputs but has JIT overhead for small ones
Comment on lines
+122
to
+126
| result = subprocess.run( | ||
| [python_path, "-m", "venv", str(venv_path)], | ||
| capture_output=True, | ||
| text=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Comment on lines
+134
to
+139
| result = subprocess.run( | ||
| [str(pip_path), "install", "-e", str(BASE_DIR), "-q"], | ||
| capture_output=True, | ||
| text=True, | ||
| cwd=str(BASE_DIR), | ||
| ) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
|
|
||
| # Warmup runs | ||
| for _ in range(warmup): | ||
| result = subprocess.run(cmd, capture_output=True, check=False) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| # Timed runs | ||
| for _ in range(iterations): | ||
| start = time.perf_counter() | ||
| result = subprocess.run(cmd, capture_output=True, check=False) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
- Add benchmarks.rst with full performance comparison - Include in documentation index for ReadTheDocs visibility
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Flags: -w/--wrapper, -r/--root, -p/--pretty, -t/--type,
-i/--item-wrap, -x/--xpath, -c/--cdata, -l/--list-headers,
-u/--url, -s/--string, -o/--output, -v/--version, -h/--help
Summary by Sourcery
Add a Python command-line interface for json2xml with parity to the Go tool and supporting scripts.
New Features:
Build:
Tests: