fix: improvements#285
Closed
vinitkumar wants to merge 1 commit intomasterfrom
Closed
Conversation
Contributor
Reviewer's GuideRefines JSON input reading, URL error handling, and dict-to-XML conversion semantics (including falsy values and special @attrs/@Val behavior), aligns the Rust accelerator with Python name normalization, and tightens CLI/benchmark/docs behavior and types. Sequence diagram for CLI input reading and unified error handlingsequenceDiagram
actor User
participant CLI
participant Utils
participant StdErr
User->>CLI: invoke main with args
CLI->>CLI: create_parser()
CLI->>CLI: parse_args()
CLI->>CLI: read_input(args)
alt URL input
CLI->>Utils: readfromurl(args.url)
alt URL read succeeds
Utils-->>CLI: JSONValue
else URL read fails
Utils-->>CLI: raise URLReadError
CLI->>CLI: exit_with_error(message)
CLI->>StdErr: print error message
CLI-->>User: SystemExit(1)
end
else string input
CLI->>Utils: readfromstring(args.string)
alt string parse succeeds
Utils-->>CLI: JSONValue
else string parse fails
Utils-->>CLI: raise StringReadError
CLI->>CLI: exit_with_error(message)
CLI->>StdErr: print error message
CLI-->>User: SystemExit(1)
end
else file input
CLI->>Utils: readfromjson(args.input_file)
alt file read succeeds
Utils-->>CLI: JSONValue
else file read fails
Utils-->>CLI: raise JSONReadError
CLI->>CLI: exit_with_error(message)
CLI->>StdErr: print error message
CLI-->>User: SystemExit(1)
end
else stdin input
CLI->>CLI: read_from_stdin()
alt stdin has data
CLI->>Utils: readfromstring(json_str)
alt stdin JSON ok
Utils-->>CLI: JSONValue
else stdin JSON invalid
Utils-->>CLI: raise StringReadError
CLI->>CLI: exit_with_error(message)
CLI->>StdErr: print error message
CLI-->>User: SystemExit(1)
end
else stdin empty
CLI->>CLI: exit_with_error("Error: Empty input")
CLI->>StdErr: print error message
CLI-->>User: SystemExit(1)
end
else no input provided
CLI->>CLI: exit_with_error("No input provided")
CLI->>StdErr: print error message
CLI-->>User: SystemExit(1)
end
Class diagram for JSONValue, input readers, serializer, and CLIclassDiagram
class JSONValue {
<<typealias>>
None
bool
int
float
str
list~JSONValue~
dict~str, JSONValue~
}
class JSONReadError {
+JSONReadError(message)
}
class URLReadError {
+URLReadError(message)
}
class StringReadError {
+StringReadError(message)
}
Exception <|-- JSONReadError
Exception <|-- URLReadError
Exception <|-- StringReadError
class Utils {
+readfromjson(filename str) JSONValue
+readfromurl(url str, params dict~str,str~) JSONValue
+readfromstring(jsondata object) JSONValue
}
class Json2xml {
-data JSONValue
-wrapper str
-root bool
-pretty bool
-attr_type bool
-item_wrap bool
-cdata bool
-list_headers bool
+Json2xml(data JSONValue, wrapper str, root bool, pretty bool, attr_type bool, item_wrap bool, cdata bool, list_headers bool)
+to_xml() Any
}
class DictToXmlEngine {
+escape_xml(s str|int|float|numbers_Number|None) str
+get_xml_type(val Any) str
+make_valid_xml_name(key str, attr dict~str,Any~) tuple~str,dict~
+dict2xml_str(item Any, item_name str, item_wrap bool, ids bool, attr_type bool, cdata bool, item_func function, list_headers bool) str
+convert_dict(obj dict~str,Any~, parent str, ids bool, attr_type bool, cdata bool, item_func function, item_wrap bool, list_headers bool) str
}
class CLI {
+exit_with_error(message str) NoReturn
+create_parser() ArgumentParser
+read_input(args Namespace) JSONValue
+read_from_stdin() JSONValue
+write_output(output str|bytes, output_file str) void
}
JSONValue <.. Utils : uses
JSONValue <.. Json2xml : uses
JSONValue <.. CLI : uses
Utils ..> JSONReadError : raises
Utils ..> URLReadError : raises
Utils ..> StringReadError : raises
Json2xml ..> DictToXmlEngine : calls dicttoxml
CLI ..> Utils : reads JSON input
CLI ..> Json2xml : converts JSON to XML
CLI ..> DictToXmlEngine : via library API
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #285 +/- ##
==========================================
+ Coverage 95.93% 96.07% +0.14%
==========================================
Files 5 6 +1
Lines 467 484 +17
==========================================
+ Hits 448 465 +17
Misses 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| # @lat: [[behavior#Input readers]] | ||
| def read_input(args: argparse.Namespace) -> dict[str, Any] | list[Any]: | ||
| def read_input(args: argparse.Namespace) -> JSONValue: |
![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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="json2xml/dicttoxml.py" line_range="340-346" />
<code_context>
attr["type"] = get_xml_type(item)
- val_attr: dict[str, str] = item.pop("@attrs", attr) # update attr with custom @attr if exists
- rawitem = item["@val"] if "@val" in item else item
+ val_attr = dict(item["@attrs"]) if "@attrs" in item else dict(attr)
+ if "@val" in item:
+ rawitem = item["@val"]
+ elif "@attrs" in item:
+ rawitem = {key: value for key, value in item.items() if key != "@attrs"}
+ else:
+ rawitem = item
if is_primitive_type(rawitem):
- if isinstance(rawitem, dict):
</code_context>
<issue_to_address>
**issue (bug_risk):** Custom `@attrs` now overwrite, rather than extend, the auto-generated attributes (e.g. `type`).
The previous `item.pop("@attrs", attr)` merged custom attributes into `attr` (including the auto-generated `type`), whereas the new code replaces `attr` with `item["@attrs"]`, dropping defaults such as `type`. If you want `@attrs` to override specific keys while keeping auto-generated ones, you could merge instead:
```python
val_attr = dict(attr)
if "@attrs" in item:
val_attr.update(item["@attrs"])
```
This retains the prior behavior while still allowing overrides.
</issue_to_address>
### Comment 2
<location path="tests/test_dict2xml.py" line_range="680-689" />
<code_context>
+ def test_dicttoxml_val_none_emits_empty_element(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for objects with `@attrs` but without `@val` to cover the branch that rebuilds `rawitem` from non‑`@attrs` keys
You already cover `@val` being `None`, booleans, and the case where `@attrs` and `@val` both exist. There’s still a branch in `dict2xml_str` where `"@attrs" in item` but `"@val"` is missing, and `rawitem` is built from the remaining keys. A test like `{"node": {"@attrs": {"id": "1"}, "child": "value"}}` (with `root=False`, `attr_type=False`) would exercise that path, confirm the `id` attribute is applied, ensure `child` renders as a nested element, and verify the input dict is not mutated.
</issue_to_address>
### Comment 3
<location path="tests/test_rust_dicttoxml.py" line_range="364-361" />
<code_context>
assert xmldata is None
+ # @lat: [[tests#Conversion behavior#Falsy JSON values convert to XML]]
+ @pytest.mark.parametrize(
+ ("data", "expected"),
+ [
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding Rust/Python parity cases for keys using the `@flat` suffix
Since `convert_dict` now treats `@flat` keys specially, please add a few such cases (both scalar and nested dicts) to the parity tests so Rust and Python behavior stays aligned, e.g. `{"name@flat": "Bike"}` and `{"item@flat": {"name": "Bike"}}`.
</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
+340
to
+346
| val_attr = dict(item["@attrs"]) if "@attrs" in item else dict(attr) | ||
| if "@val" in item: | ||
| rawitem = item["@val"] | ||
| elif "@attrs" in item: | ||
| rawitem = {key: value for key, value in item.items() if key != "@attrs"} | ||
| else: | ||
| rawitem = item |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Custom @attrs now overwrite, rather than extend, the auto-generated attributes (e.g. type).
The previous item.pop("@attrs", attr) merged custom attributes into attr (including the auto-generated type), whereas the new code replaces attr with item["@attrs"], dropping defaults such as type. If you want @attrs to override specific keys while keeping auto-generated ones, you could merge instead:
val_attr = dict(attr)
if "@attrs" in item:
val_attr.update(item["@attrs"])This retains the prior behavior while still allowing overrides.
Comment on lines
+680
to
+689
| def test_dicttoxml_val_none_emits_empty_element(self) -> None: | ||
| """Test @val=None serializes as empty text without leaking Python's repr.""" | ||
| result = dicttoxml.dicttoxml( | ||
| {"field": {"@attrs": {"source": "api"}, "@val": None}}, | ||
| root=False, | ||
| attr_type=False, | ||
| ) | ||
|
|
||
| assert result == b'<field source="api"></field>' | ||
| assert b"None" not in result |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test for objects with @attrs but without @val to cover the branch that rebuilds rawitem from non‑@attrs keys
You already cover @val being None, booleans, and the case where @attrs and @val both exist. There’s still a branch in dict2xml_str where "@attrs" in item but "@val" is missing, and rawitem is built from the remaining keys. A test like {"node": {"@attrs": {"id": "1"}, "child": "value"}} (with root=False, attr_type=False) would exercise that path, confirm the id attribute is applied, ensure child renders as a nested element, and verify the input dict is not mutated.
| @@ -360,6 +360,27 @@ def test_very_large_integer_matches(self): | |||
| rust, python = self.compare_outputs(data) | |||
| assert rust == python | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding Rust/Python parity cases for keys using the @flat suffix
Since convert_dict now treats @flat keys specially, please add a few such cases (both scalar and nested dicts) to the parity tests so Rust and Python behavior stays aligned, e.g. {"name@flat": "Bike"} and {"item@flat": {"name": "Bike"}}.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by Sourcery
Improve JSON input readers and XML conversion behavior, expand tests for realistic HTTP and edge cases, and refresh documentation and benchmarks to reflect current performance and semantics.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: