[python] Add mock API tests for XML scenarios from PR #10063 and fix XML serialization bugs#10098
[python] Add mock API tests for XML scenarios from PR #10063 and fix XML serialization bugs#10098
Conversation
Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com>
Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com>
commit: |
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
packages/http-client-python/generator/test/generic_mock_api_tests/test_payload_xml.py
Outdated
Show resolved
Hide resolved
|
@copilot analyze the review comment #10098 (review). |
…cks, improve namespace_map comment, remove .venv_test from .gitignore Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/0b94f8d4-fb48-435f-a266-158971884321
Addressed in e02d171:
All four points from the review have been addressed in e02d171. See the reply on the review comment for details. |
msyyc
left a comment
There was a problem hiding this comment.
Review: model_base.py.jinja2 - duplicated namespace fallback logic
The bug fixes in this PR (namespace key compat, ET.register_namespace ValueError handling, items naming) are all correct. The main concern is structural cleanliness.
Core issue: 9x duplicated 'ns vs namespace' fallback pattern
The PR copy-pastes this 2-4 line block nine times across the file:
xml_ns = something.get('ns')
if xml_ns is None:
xml_ns = something.get('namespace')at lines 633-635, 639-641, 774-776, 778-780, 1278-1280, 1322-1323, 1346-1348, 1365-1369, and 1388-1391.
Suggestion: Extract a small helper to eliminate the repetition:
def _get_xml_ns(meta: dict) -> typing.Optional[str]:
ns = meta.get('ns')
if ns is None:
ns = meta.get('namespace')
return nsThis collapses all 9 sites into one-liners, and the two identical prop-to-model namespace resolution blocks (deserialization at lines 633-641 and discriminator at lines 774-780) into:
xml_ns = _get_xml_ns(prop_meta)
if xml_ns is None and not model_meta.get('prefix'):
xml_ns = _get_xml_ns(model_meta)| Issue | Severity |
|---|---|
| 9x duplicated ns/namespace fallback - extract helper | High (readability/maintainability) |
| Identical blocks at lines 633-641 and 774-780 - DRY violation | Medium |
| Future key rename requires 9 edits instead of 1 | Medium (fragility) |
|
@copilot fix with review comment #10098 (review) |
…mespace fallback pattern Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/c063fa73-5d6e-4cbd-ae5b-c5a2d188abba
Extracted xml_ns = _get_xml_ns(prop_meta)
if xml_ns is None and not model_meta.get("prefix"):
xml_ns = _get_xml_ns(model_meta)The old-style
Done in 460cfdb — extracted |
Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com>
|
@copilot fix the following CI error: |
…rialization Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/bacb0178-de28-436e-81a5-8ecd967cc74d Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com>
... Fixed in ab1635e. The mypy error was caused by |
itemNs→itemsNskey mismatch in deserialization (red bug)orchains for namespace lookups withis Nonechecks (handles empty-string namespaces correctly)ET._namespace_mapsilent fallback behavior.venv_testfrom.gitignore(local dev leftover)_get_xml_nshelper to eliminate 9x duplicated ns/namespace fallback patternOptional[Any]type error in unwrapped array deserialization blockOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.