Remove some legacy Python compatible codes #244
Conversation
WalkthroughThe recent updates focus on simplifying the test suite and enhancing Python version compatibility within the library. By removing unnecessary imports and version-specific conditions, the changes streamline the testing process. Additionally, the modifications to Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (5)
- tests/conftest.py (1 hunks)
- tests/test_framed_transport.py (3 hunks)
- tests/test_tornado.py (7 hunks)
- thriftpy2/hook.py (2 hunks)
- thriftpy2/parser/parser.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/conftest.py
Additional comments: 3
thriftpy2/hook.py (1)
- 5-6: The addition of
importlib.abcandimportlib.utilis a positive change, aligning with the PR's objective to modernize the codebase by utilizing more current Python module loading techniques.tests/test_framed_transport.py (1)
- 2-7: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-100]
The removal of
sysandpytestimports, as well aspytest.mark.skipifdecorators, fromtests/test_framed_transport.pysimplifies the test setup by eliminating version-specific conditions. This change is consistent with the PR's objective to modernize the codebase. Ensure that the test suite still covers all necessary scenarios without relying on version-specific checks.tests/test_tornado.py (1)
- 10-15: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-100]
Removing
pytestimports and@pytest.mark.skipifdecorators fromtests/test_tornado.pysimplifies the test setup by eliminating version-specific skips. This is a positive change towards modernizing the test suite. However, it's important to verify that the removal of these conditions does not inadvertently exclude important test scenarios that are still relevant. Ensure comprehensive test coverage is maintained.
| cached, this is enabled by default. If `module_name` | ||
| is provided, use it as cache key, else use the `path`. | ||
| """ | ||
| if os.name == 'nt' and sys.version_info[0] < 3: | ||
| os.path.samefile = lambda f1, f2: os.stat(f1) == os.stat(f2) | ||
|
|
||
| # dead include checking on current stack | ||
| for thrift in thrift_stack: | ||
| if thrift.__thrift_file__ is not None and \ |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-22]
The import of PY3 from thriftpy2._compat is no longer necessary if the goal is to remove legacy Python compatibility code. Since Python 2 support is being dropped, all code that is conditional on PY3 or Python 2 should be removed or simplified.
- from thriftpy2._compat import urlopen, urlparse, PY3
+ from thriftpy2._compat import urlopen, urlparseAnd remove any usage of PY3 throughout the file.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1013-1015]
Given the removal of Python 2 support, the conditional check for PY3 to decode bytes to a string is no longer necessary. All code can now assume Python 3 behavior, simplifying the handling of data read from files or URLs.
- if PY3:
- if isinstance(data, bytes):
- data = data.decode(encoding)This block can be removed, assuming all environments will be running Python 3 or later.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [27-29]
The conditional import of open from io for Python versions not equal to PY3 is unnecessary with the removal of Python 2 support. This can be removed to simplify the codebase.
- if not PY3:
- from io import openRemoving this will also eliminate the need for the PY3 variable, further simplifying the code.
| class ThriftImporter(importlib.abc.MetaPathFinder): | ||
| def __init__(self, extension="_thrift"): | ||
| self.extension = extension | ||
|
|
||
| class ThriftImporter(importlib.abc.MetaPathFinder): | ||
| def __init__(self, extension="_thrift"): | ||
| self.extension = extension | ||
| def find_spec(self, fullname, path, target=None): | ||
| if not fullname.endswith(self.extension): | ||
| return None | ||
| return importlib.util.spec_from_loader(fullname, | ||
| ThriftLoader(fullname)) |
There was a problem hiding this comment.
The ThriftImporter class has been updated to implement the importlib.abc.MetaPathFinder interface. This is a good practice for custom module importers, ensuring compatibility with Python's import system. However, the find_spec method lacks documentation. Adding a docstring explaining its purpose, parameters, and return value would enhance maintainability.
+ def find_spec(self, fullname, path, target=None):
+ """
+ Find a module's spec based on its fullname, path, and target.
+
+ :param fullname: Full name of the module.
+ :param path: Path to the module, if available.
+ :param target: Target module, if available.
+ :return: Module spec if found, None otherwise.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| class ThriftImporter(importlib.abc.MetaPathFinder): | |
| def __init__(self, extension="_thrift"): | |
| self.extension = extension | |
| class ThriftImporter(importlib.abc.MetaPathFinder): | |
| def __init__(self, extension="_thrift"): | |
| self.extension = extension | |
| def find_spec(self, fullname, path, target=None): | |
| if not fullname.endswith(self.extension): | |
| return None | |
| return importlib.util.spec_from_loader(fullname, | |
| ThriftLoader(fullname)) | |
| class ThriftImporter(importlib.abc.MetaPathFinder): | |
| def __init__(self, extension="_thrift"): | |
| self.extension = extension | |
| def find_spec(self, fullname, path, target=None): | |
| """ | |
| Find a module's spec based on its fullname, path, and target. | |
| :param fullname: Full name of the module. | |
| :param path: Path to the module, if available. | |
| :param target: Target module, if available. | |
| :return: Module spec if found, None otherwise. | |
| """ | |
| if not fullname.endswith(self.extension): | |
| return None | |
| return importlib.util.spec_from_loader(fullname, | |
| ThriftLoader(fullname)) |
| class ThriftLoader(importlib.abc.Loader): | ||
| def __init__(self, fullname): | ||
| self.fullname = fullname | ||
|
|
||
| class ThriftLoader(importlib.abc.Loader): | ||
| def __init__(self, fullname): | ||
| self.fullname = fullname | ||
| def create_module(self, spec): | ||
| return load_module(self.fullname) | ||
|
|
||
| def create_module(self, spec): | ||
| return load_module(self.fullname) | ||
|
|
||
| def exec_module(self, module): | ||
| pass | ||
| else: | ||
| class ThriftImporter(object): | ||
| def __init__(self, extension="_thrift"): | ||
| self.extension = extension | ||
|
|
||
| def find_module(self, fullname, path=None): | ||
| if fullname.endswith(self.extension): | ||
| return self | ||
|
|
||
| def load_module(self, fullname): | ||
| return load_module(fullname) | ||
| def exec_module(self, module): | ||
| pass |
There was a problem hiding this comment.
The ThriftLoader class correctly implements the importlib.abc.Loader interface. The create_module method is well-defined, but like ThriftImporter, it could benefit from additional documentation for clarity and maintainability.
+ def create_module(self, spec):
+ """
+ Create a module based on the provided spec.
+
+ :param spec: Specification of the module to be created.
+ :return: The loaded module.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| class ThriftLoader(importlib.abc.Loader): | |
| def __init__(self, fullname): | |
| self.fullname = fullname | |
| class ThriftLoader(importlib.abc.Loader): | |
| def __init__(self, fullname): | |
| self.fullname = fullname | |
| def create_module(self, spec): | |
| return load_module(self.fullname) | |
| def create_module(self, spec): | |
| return load_module(self.fullname) | |
| def exec_module(self, module): | |
| pass | |
| else: | |
| class ThriftImporter(object): | |
| def __init__(self, extension="_thrift"): | |
| self.extension = extension | |
| def find_module(self, fullname, path=None): | |
| if fullname.endswith(self.extension): | |
| return self | |
| def load_module(self, fullname): | |
| return load_module(fullname) | |
| def exec_module(self, module): | |
| pass | |
| class ThriftLoader(importlib.abc.Loader): | |
| def __init__(self, fullname): | |
| self.fullname = fullname | |
| def create_module(self, spec): | |
| """ | |
| Create a module based on the provided spec. | |
| :param spec: Specification of the module to be created. | |
| :return: The loaded module. | |
| """ | |
| return load_module(self.fullname) | |
| def exec_module(self, module): | |
| pass |
| # TODO: The load process does not compatible with Python standard, e.g., if the | ||
| # specified thrift file does not exists, it raises FileNotFoundError, and skiped | ||
| # the other meta finders in the sys.meta_path. | ||
| if sys.version_info >= (3, 4): | ||
| import importlib.abc | ||
| import importlib.util | ||
| class ThriftImporter(importlib.abc.MetaPathFinder): | ||
| def __init__(self, extension="_thrift"): | ||
| self.extension = extension | ||
|
|
||
| class ThriftImporter(importlib.abc.MetaPathFinder): | ||
| def __init__(self, extension="_thrift"): | ||
| self.extension = extension | ||
| def find_spec(self, fullname, path, target=None): | ||
| if not fullname.endswith(self.extension): | ||
| return None | ||
| return importlib.util.spec_from_loader(fullname, | ||
| ThriftLoader(fullname)) | ||
|
|
||
| def find_spec(self, fullname, path, target=None): | ||
| if not fullname.endswith(self.extension): | ||
| return None | ||
| return importlib.util.spec_from_loader(fullname, | ||
| ThriftLoader(fullname)) | ||
|
|
||
| class ThriftLoader(importlib.abc.Loader): | ||
| def __init__(self, fullname): | ||
| self.fullname = fullname | ||
|
|
||
| class ThriftLoader(importlib.abc.Loader): | ||
| def __init__(self, fullname): | ||
| self.fullname = fullname | ||
| def create_module(self, spec): | ||
| return load_module(self.fullname) | ||
|
|
||
| def create_module(self, spec): | ||
| return load_module(self.fullname) | ||
|
|
||
| def exec_module(self, module): | ||
| pass | ||
| else: | ||
| class ThriftImporter(object): | ||
| def __init__(self, extension="_thrift"): | ||
| self.extension = extension | ||
|
|
||
| def find_module(self, fullname, path=None): | ||
| if fullname.endswith(self.extension): | ||
| return self | ||
|
|
||
| def load_module(self, fullname): | ||
| return load_module(fullname) | ||
| def exec_module(self, module): | ||
| pass | ||
|
|
||
|
|
||
| _imp = ThriftImporter() |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [5-34]
The TODO comment at the beginning of the file raises a concern about the load process not being compatible with the Python standard, specifically mentioning that a FileNotFoundError is raised, skipping other meta finders in sys.meta_path. This is a significant issue that could affect the robustness and predictability of the custom module loading mechanism. It's recommended to address this issue or at least document the current behavior and its implications clearly in the code.
Consider revising the loading process to adhere more closely to Python's standard behavior, or document the current behavior and its implications to ensure users are aware of this deviation.
|
I think we should keep this PR small to make it easy to review. So the removal of |
Summary by CodeRabbit