Conversation
xml_namespaces had a mutable default dict which is a classic Python
footgun where mutations persist across calls. Changed to None with
an 'or {}' guard inside the function body, matching the pattern
already used in dicttoxml_fast.py.
Amp-Thread-ID: https://ampcode.com/threads/T-019d6bf2-ce86-763d-8c62-08118f358cbe
Co-authored-by: Amp <amp@ampcode.com>
These typing imports were dead code since the file already uses the built-in dict[...] and list[...] syntax everywhere. Amp-Thread-ID: https://ampcode.com/threads/T-019d6bf2-ce86-763d-8c62-08118f358cbe Co-authored-by: Amp <amp@ampcode.com>
These were unittest-style no-op methods that serve no purpose in pytest classes. Amp-Thread-ID: https://ampcode.com/threads/T-019d6bf2-ce86-763d-8c62-08118f358cbe Co-authored-by: Amp <amp@ampcode.com>
Avoids creating a new SystemRandom instance on every make_id() call. The module-level _SAFE_RANDOM is reused across all invocations. Amp-Thread-ID: https://ampcode.com/threads/T-019d6bf2-ce86-763d-8c62-08118f358cbe Co-authored-by: Amp <amp@ampcode.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCentralizes use of SystemRandom for ID generation and fixes a mutable default argument in dicttoxml’s xml_namespaces parameter, plus minor test cleanup. Class diagram for updated dicttoxml module structureclassDiagram
class DicttoxmlModule {
+SystemRandom _SAFE_RANDOM
+str make_id(element, start, end)
+bytes dicttoxml(obj, root, ids, attr_type, item_wrap, item_func, cdata, xml_namespaces, list_headers, xpath_format)
+str get_unique_id(element)
}
class SystemRandom {
+int randint(start, end)
}
DicttoxmlModule --> SystemRandom : uses _SAFE_RANDOM
DicttoxmlModule ..> SystemRandom : make_id uses randint()
File-Level Changes
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 #281 +/- ##
==========================================
+ Coverage 95.91% 95.93% +0.01%
==========================================
Files 5 5
Lines 465 467 +2
==========================================
+ Hits 446 448 +2
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. |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="json2xml/dicttoxml.py" line_range="800" />
<code_context>
item_func: Callable[[str], str] = default_item_func,
cdata: bool = False,
- xml_namespaces: dict[str, Any] = {},
+ xml_namespaces: dict[str, Any] | None = None,
list_headers: bool = False,
xpath_format: bool = False,
</code_context>
<issue_to_address>
**suggestion:** Avoid using `or {}` so that only `None` is treated as the "no namespaces" case
`xml_namespaces = xml_namespaces or {}` will replace any falsy-but-valid value (including custom mapping-like objects with `__bool__` returning `False`) with `{}`, which can be surprising. Prefer an explicit `None` check instead:
```python
if xml_namespaces is None:
xml_namespaces = {}
```
This preserves existing behavior for `None` while not altering other valid inputs.
```suggestion
if xml_namespaces is None:
xml_namespaces = {}
```
</issue_to_address>
### Comment 2
<location path="tests/test_json2xml.py" line_range="21" />
<code_context>
class TestJson2xml:
"""Tests for `json2xml` package."""
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests covering the updated `xml_namespaces` behavior in `dicttoxml` (None default, no shared mutation, and explicit namespaces).
Since `xml_namespaces` changed from a mutable default (`{}`) to an optional argument (`None` with `xml_namespaces = xml_namespaces or {}`), tests should explicitly cover:
1. Calling `dicttoxml` without `xml_namespaces` and confirming the output matches previous behavior (no unexpected namespace attributes).
2. Calling `dicttoxml` with an explicit `xml_namespaces` dict (including `xsi` with multiple schema attributes) and asserting the XML has the expected namespace declarations and attributes.
3. Reusing the same `xml_namespaces` dict across multiple calls and asserting the dict is unchanged and namespaces don’t accumulate.
These will guard against regressions in the new default-handling logic and verify the mutable-default fix is behaviorally transparent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -21,12 +21,6 @@ | |||
| class TestJson2xml: | |||
There was a problem hiding this comment.
suggestion (testing): Add tests covering the updated xml_namespaces behavior in dicttoxml (None default, no shared mutation, and explicit namespaces).
Since xml_namespaces changed from a mutable default ({}) to an optional argument (None with xml_namespaces = xml_namespaces or {}), tests should explicitly cover:
- Calling
dicttoxmlwithoutxml_namespacesand confirming the output matches previous behavior (no unexpected namespace attributes). - Calling
dicttoxmlwith an explicitxml_namespacesdict (includingxsiwith multiple schema attributes) and asserting the XML has the expected namespace declarations and attributes. - Reusing the same
xml_namespacesdict across multiple calls and asserting the dict is unchanged and namespaces don’t accumulate.
These will guard against regressions in the new default-handling logic and verify the mutable-default fix is behaviorally transparent.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Summary by Sourcery
Reuse a module-level secure random generator and correct the handling of optional XML namespace configuration in dicttoxml while cleaning up unused test fixtures.
Bug Fixes:
Enhancements:
Tests: