-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed Compatiblity Issues with Python 3.9 #18
Conversation
f55b3ae
to
709ca38
Compare
709ca38
to
cd18e93
Compare
Reviewer's Guide by SourceryThis pull request addresses compatibility issues with Python 3.9 and updates the supported Python versions. The main changes involve updating type hints, refactoring a decorator, and modifying the project configuration. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @stegm - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -30,7 +30,7 @@ def client_response_factory( | |||
) -> Callable[[int, Any], MagicMock]: | |||
"""Provides a factory to add responses to a ClientSession.""" | |||
|
|||
def factory(status: int = 200, json: list[Any] | dict[Any, Any] | None = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Update type hint in client_response_factory for Python 3.9 compatibility
This change is necessary for Python 3.9 compatibility. Consider adding a test case that specifically checks if the factory works correctly with different types of input (list, dict, and None) to ensure the change doesn't introduce any regressions.
def factory(status: int = 200, json: list[Any] | dict[Any, Any] | None = None): | |
def factory( | |
status: int = 200, | |
json: Union[list[Any], dict[Any, Any], None] = None | |
) -> MagicMock: |
@@ -40,7 +40,9 @@ | |||
async def test_virtual_process_data( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add test for Python 3.9 specific behavior
Since this PR is about fixing compatibility issues with Python 3.9, it would be beneficial to add a test case that specifically checks for correct behavior under Python 3.9, especially regarding the changes made to type hints and any other Python 3.9 specific modifications.
from typing import Any, Callable, Iterable, Union
from unittest.mock import ANY, MagicMock, call
import sys
import pytest
@pytest.mark.skipif(sys.version_info < (3, 9), reason="requires python3.9 or higher")
async def test_virtual_process_data_python39():
# Test implementation for Python 3.9 specific behavior
1db7361
to
8176a84
Compare
This seems to work for me on a system that only has Python 3.9. |
According to #17, the code does not run with python 3.9. I updated tox to include 3.9 and also 3.12. I dropped 3.8 because it never worked and support for python will drop in October.
This PR contains code changes which makes it compatible with 3.9. Most of the changes fix the new typing syntax. In api.py the decorator for re-login has to be moved into a function instead a static method.