fix: cleanup code #248
fix: cleanup code #248
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR removes redundant type-only imports from test modules and tightens linting rules by re-enabling unused-import checks. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #248 +/- ##
===========================================
+ Coverage 99.30% 100.00% +0.69%
===========================================
Files 3 3
Lines 288 288
===========================================
+ Hits 286 288 +2
+ Misses 2 0 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
tests/test_dict2xml.py
Outdated
|
|
||
| if TYPE_CHECKING: | ||
| from _pytest.capture import CaptureFixture | ||
| from _pytest.fixtures import FixtureRequest | ||
| from _pytest.logging import LogCaptureFixture | ||
| from _pytest.monkeypatch import MonkeyPatch | ||
| from pytest_mock.plugin import MockerFixture | ||
| pass |
There was a problem hiding this comment.
suggestion (code-quality): Remove redundant conditional (remove-redundant-if)
| if TYPE_CHECKING: | |
| from _pytest.capture import CaptureFixture | |
| from _pytest.fixtures import FixtureRequest | |
| from _pytest.logging import LogCaptureFixture | |
| from _pytest.monkeypatch import MonkeyPatch | |
| from pytest_mock.plugin import MockerFixture | |
| pass |
tests/test_utils.py
Outdated
| if TYPE_CHECKING: | ||
| from _pytest.capture import CaptureFixture | ||
| from _pytest.fixtures import FixtureRequest | ||
| from _pytest.logging import LogCaptureFixture | ||
| from _pytest.monkeypatch import MonkeyPatch | ||
| from pytest_mock.plugin import MockerFixture | ||
| pass |
There was a problem hiding this comment.
issue (code-quality): Remove redundant conditional (remove-redundant-if)
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up test code and improves code quality by simplifying TYPE_CHECKING blocks and enforcing unused import detection. It removes redundant type imports that were only used for type checking and enables stricter linting.
- Simplifies TYPE_CHECKING blocks by replacing type imports with pass statements
- Removes actual unused imports from test files
- Enables F401 linting rule to catch unused imports in the future
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_utils.py | Replaces TYPE_CHECKING imports with pass statement |
| tests/test_json2xml.py | Removes unused json import |
| tests/test_dict2xml.py | Removes unused numbers import and replaces TYPE_CHECKING imports with pass |
| pyproject.toml | Removes F401 from ignored lint rules to enforce unused import detection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tests/test_utils.py
Outdated
| if TYPE_CHECKING: | ||
| from _pytest.capture import CaptureFixture | ||
| from _pytest.fixtures import FixtureRequest | ||
| from _pytest.logging import LogCaptureFixture | ||
| from _pytest.monkeypatch import MonkeyPatch | ||
| from pytest_mock.plugin import MockerFixture | ||
| pass |
There was a problem hiding this comment.
Empty TYPE_CHECKING blocks with only 'pass' serve no purpose and add unnecessary code. Consider removing the entire TYPE_CHECKING block if no type imports are needed.
Copilot uses AI. Check for mistakes.
tests/test_dict2xml.py
Outdated
| if TYPE_CHECKING: | ||
| from _pytest.capture import CaptureFixture | ||
| from _pytest.fixtures import FixtureRequest | ||
| from _pytest.logging import LogCaptureFixture | ||
| from _pytest.monkeypatch import MonkeyPatch | ||
| from pytest_mock.plugin import MockerFixture | ||
| pass |
There was a problem hiding this comment.
Empty TYPE_CHECKING blocks with only 'pass' serve no purpose and add unnecessary code. Consider removing the entire TYPE_CHECKING block if no type imports are needed.
Copilot uses AI. Check for mistakes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def test_get_unique_id_collision_coverage(self) -> None: | ||
| """Test get_unique_id to cover line 50 - the collision case.""" | ||
| import json2xml.dicttoxml as module |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
The recommended fix is to remove the from json2xml import dicttoxml import (line 6) from the file and instead use a plain module import (import json2xml.dicttoxml) at the top level. Any use of dicttoxml throughout the test class will then need to be updated to reference json2xml.dicttoxml. This ensures there is only one import style for the module, as recommended. To preserve existing functionality, replace any call or reference to dicttoxml (as a module) with json2xml.dicttoxml (or another suitable alias if desired, but the most consistent choice is the fully-qualified name). Note that in this file, all the uses of dicttoxml.funcname and dicttoxml.dicttoxml, etc., will need to be adjusted accordingly.
All changes are limited to tests/test_dict2xml.py, affecting:
- The import statements at the top of the file,
- All instances where
dicttoxmlmodule is referenced (e.g.,dicttoxml.dicttoxml,dicttoxml.get_unique_id, etc.).
No new imports are needed except the top-level import json2xml.dicttoxml. No changes to functionality are required outside changing the way the module is referenced.
| @@ -3,7 +3,7 @@ | ||
|
|
||
| import pytest | ||
|
|
||
| from json2xml import dicttoxml | ||
| import json2xml.dicttoxml | ||
|
|
||
|
|
||
| class TestDict2xml: | ||
| @@ -1122,7 +1122,7 @@ | ||
| def test_attrs_empty_and_none_values(self) -> None: | ||
| """Test attribute handling with empty and None values.""" | ||
| data = {"Element": {"@attrs": {"empty": "", "zero": 0, "false": False}}} | ||
| result = dicttoxml.dicttoxml( | ||
| result = json2xml.dicttoxml.dicttoxml( | ||
| data, attr_type=False, item_wrap=False, root=False | ||
| ).decode("utf-8") | ||
|
|
||
| @@ -1177,14 +1177,14 @@ | ||
|
|
||
| try: | ||
| # First call - adds "test_123456" to _used_ids | ||
| result1 = dicttoxml.get_unique_id("test") | ||
| result1 = json2xml.dicttoxml.get_unique_id("test") | ||
| assert result1 == "test_123456" | ||
|
|
||
| # Reset call count for second test | ||
| call_count = 0 | ||
|
|
||
| # Second call - should trigger collision and regenerate | ||
| result2 = dicttoxml.get_unique_id("test") | ||
| result2 = json2xml.dicttoxml.get_unique_id("test") | ||
| assert result2 == "test_789012" | ||
| assert call_count == 3 # Should have called make_id 3 times | ||
| finally: |
Summary by Sourcery
Clean up test code and lint configuration by removing redundant type imports in TYPE_CHECKING blocks and re-enabling unused import detection.
Enhancements:
Build: