Feat/attr escape #224
Feat/attr escape #224
Conversation
Reviewer's Guide by SourceryThis PR implements XML attribute escaping functionality by modifying the attribute string generation process and adding corresponding test cases. The changes ensure that special characters in XML attributes are properly escaped to maintain valid XML structure. Sequence diagram for XML attribute escaping processsequenceDiagram
participant Client
participant DictToXml
participant EscapeXml
Client->>DictToXml: Call make_attrstring(attr)
DictToXml->>EscapeXml: Call escape_xml(value)
EscapeXml-->>DictToXml: Return escaped value
DictToXml-->>Client: Return attribute string with escaped values
Class diagram for updated attribute string generationclassDiagram
class DictToXml {
+make_attrstring(attr: dict[str, Any]) str
}
class EscapeXml {
+escape_xml(value: str) str
}
DictToXml --|> EscapeXml : uses
note for DictToXml "Updated to use escape_xml for attribute values"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vinitkumar - I've reviewed your changes - here's some feedback:
Overall Comments:
- Good addition of XML attribute escaping, but please fix the failing test cases. The escaping needs to work consistently across both the json2xml and dicttoxml conversion paths.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for i, data in enumerate(test_cases): | ||
| print(f"\nTest case {i + 1}:") | ||
| print("Input:", data) | ||
| xml = dicttoxml.dicttoxml(data, custom_root='all') | ||
| print("Output XML:") | ||
| print(xml.decode('utf-8')) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
❌ 4 Tests Failed:
View the full list of 3 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| import pytest | |||
|
|
|||
| from json2xml import dicttoxml | |||
| from json2xml import dicttoxml, json2xml | |||
Check notice
Code scanning / CodeQL
Unused import Note test
| @@ -0,0 +1,56 @@ | |||
| import pytest | |||
Check notice
Code scanning / CodeQL
Unused import Note test
Summary by Sourcery
Implement XML attribute escaping in the dicttoxml module to ensure special characters are properly handled, and add corresponding test cases to validate this functionality.
New Features:
Tests: