Conversation
- remove python 2.7 support - add type hints - better comments - passing tests
There was a problem hiding this comment.
Pull Request Overview
This pull request (v1.1.0) prepares the library for Python 3.8+ by removing Python 2.7 support, adding type hints throughout the codebase, and refactoring various components including C library loading and documentation.
- Updated type annotations and formatting for improved readability and reliability
- Refactored the ctypes library loading logic and removed legacy modules (e.g., sysutils)
- Enhanced CI integration with GitHub Actions and updated documentation/examples
Reviewed Changes
Copilot reviewed 25 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ (various) | Updated test definitions and formatting |
| tasks.py, geosupport/sysutils.py | Removed legacy code |
| setup.py, geosupport/*, geosupport/config.py | Added type hints, logging, and refactored library loading |
| examples/, docs/, README.md | Updated formatting and documentation for clarity and style |
| .github/workflows/ci.yml | Added CI pipeline steps using GitHub Actions |
Files not reviewed (3)
- .devcontainer/Dockerfile: Language not supported
- .devcontainer/devcontainer.json: Language not supported
- appveyor/build.ps1: Language not supported
geosupport/geosupport.py
Outdated
| GEOLIB: Any = None | ||
|
|
There was a problem hiding this comment.
The use of a global variable 'GEOLIB' may lead to race conditions in multi-threaded or multi-instance scenarios. It is recommended to encapsulate the loaded library within the Geosupport class instance to improve thread safety.
| GEOLIB: Any = None | |
| # GEOLIB: Any = None |
geosupport/error.py
Outdated
| def __init__(self, message: str, result: Dict[str, Any] = {}) -> None: | ||
| super(GeosupportError, self).__init__(message) | ||
| self.result = result |
There was a problem hiding this comment.
Using a mutable default argument (an empty dict) in the constructor may lead to unexpected behavior. It is recommended to use a default value of None and then assign an empty dict inside the method.
| def __init__(self, message: str, result: Dict[str, Any] = {}) -> None: | |
| super(GeosupportError, self).__init__(message) | |
| self.result = result | |
| def __init__(self, message: str, result: Dict[str, Any] = None) -> None: | |
| super(GeosupportError, self).__init__(message) | |
| self.result = result if result is not None else {} |
1 participant
Uh oh!
There was an error while loading. Please reload this page.