Remove OrderKeepingDict. as of py 3.7 dict keeps order. Removed some … #189
Conversation
…py 2 compatibility which resulted in continued use of basestring
## Walkthrough
The changes remove legacy Python 2 compatibility code, update type checks to use Python 3 conventions, and replace a custom ordered dictionary with standard dicts. Resource loading is modernized, and setup metadata is refreshed. Minor test adjustments ensure compatibility with Python 3’s dictionary view semantics.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------|
| autowrap/Code.py<br>autowrap/CodeGenerator.py<br>autowrap/ConversionProvider.py<br>autowrap/PXDParser.py | Removed Python 2/3 compatibility blocks; updated all string type checks from `basestring`/`unicode` to `(str, bytes)`/`str`. |
| autowrap/DeclResolver.py<br>autowrap/PXDParser.py | Replaced `OrderKeepingDictionary` with standard `dict` for method storage and type annotations. |
| autowrap/PXDParser.py | Updated resource loading from `pkg_resources.resource_filename` to `importlib.resources.files`. |
| autowrap/tools.py | Deleted file; removed the `OrderKeepingDictionary` implementation entirely. |
| setup.py | Switched to `find_namespace_packages`, updated maintainer and license metadata, refined package inclusion/exclusion. |
| tests/test_decl_resolver.py | Changed extraction of first method from dict values to use `list()` for Python 3 compatibility. |
| tests/test_pxd_parser.py | Modified assertion to compare lists instead of dict_keys view for Python 3 compatibility. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Code
participant CodeGenerator
participant ConversionProvider
participant PXDParser
User->>Code: add() / _render()
Code->>Code: isinstance(..., (str, bytes))
User->>CodeGenerator: generate code
CodeGenerator->>CodeGenerator: isinstance(..., (str, bytes))
User->>ConversionProvider: input_conversion()
ConversionProvider->>ConversionProvider: isinstance(..., str)
User->>PXDParser: parse_pxd_file()
PXDParser->>PXDParser: importlib.resources.filesPoem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
setup.py (1)
12-12: Consider using a context manager for file handlingThe file is correctly read directly now, but it would be better to use a context manager to ensure proper file closure.
-exec(open(versionfile).read()) +with open(versionfile) as f: + exec(f.read())🧰 Tools
🪛 Ruff (0.8.2)
12-12: Use a context manager for opening files
(SIM115)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
autowrap/Code.py(2 hunks)autowrap/CodeGenerator.py(11 hunks)autowrap/ConversionProvider.py(2 hunks)autowrap/DeclResolver.py(2 hunks)autowrap/PXDParser.py(4 hunks)autowrap/tools.py(0 hunks)setup.py(2 hunks)tests/test_decl_resolver.py(1 hunks)tests/test_pxd_parser.py(1 hunks)
💤 Files with no reviewable changes (1)
- autowrap/tools.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
autowrap/CodeGenerator.py (2)
autowrap/Code.py (2)
add(55-74)Code(40-89)autowrap/ConversionProvider.py (16)
output_conversion(131-134)output_conversion(188-191)output_conversion(233-236)output_conversion(268-271)output_conversion(315-318)output_conversion(348-351)output_conversion(378-381)output_conversion(436-440)output_conversion(473-476)output_conversion(508-511)output_conversion(541-544)output_conversion(608-624)output_conversion(747-801)output_conversion(1095-1169)output_conversion(1305-1353)output_conversion(1875-1950)
🪛 Ruff (0.8.2)
setup.py
12-12: Use a context manager for opening files
(SIM115)
16-16: Undefined name __version__
(F821)
🔇 Additional comments (18)
setup.py (5)
9-9: Good update to use namespace packagesThe change from
find_packagestofind_namespace_packagesis appropriate for supporting the namespace packaging style used by the autowrap modules.
17-19: Updated project metadataUpdating the maintainer information and simplifying the license string to the SPDX identifier is a good modernization step.
30-31: Added Python 3.12 supportGood addition of Python 3.12 to the supported versions, consistent with dropping Python 2 compatibility.
37-37: Improved package inclusion patternsThe refined package patterns correctly include all autowrap namespace packages while excluding test and example code.
16-16:Details
✅ Verification successful
Fix undefined version reference
The static analysis tool has flagged a potential issue with
__version__being undefined. This variable should be coming from theversion.pyfile that was just executed, but there's no explicit verification.-version="%d.%d.%d" % __version__, +# After executing version.py, __version__ should be defined +version="%d.%d.%d" % __version__ if '__version__' in globals() else (0, 0, 0),
🏁 Script executed:
#!/bin/bash # Check how version.py defines the version cat autowrap/version.pyLength of output: 1631
🏁 Script executed:
#!/bin/bash sed -n '1,200p' setup.pyLength of output: 1640
No change needed:
__version__is defined via execThe call to
exec(open("autowrap/version.py").read())imports
__version__ = (0, 22, 12) version = __version__so
__version__is guaranteed to be defined when used inversion="%d.%d.%d" % __version__,The static analysis warning can be safely ignored.
🧰 Tools
🪛 Ruff (0.8.2)
16-16: Undefined name
__version__(F821)
tests/test_pxd_parser.py (1)
347-347: Fixed dictionary view compatibility for Python 3Good update to convert the dictionary keys view to a list before comparison. This is necessary because in Python 3,
dict.keys()returns a view object rather than a list.tests/test_decl_resolver.py (1)
795-795: Fixed dictionary values view for Python 3 compatibilityGood update to convert the dictionary values view to a list before indexing. This is necessary because in Python 3,
dict.values()returns a view object that doesn't support direct indexing.autowrap/PXDParser.py (4)
403-403: Replaced OrderKeepingDict with standard dictExcellent change to use the standard
dicttype. Since Python 3.7+, the built-in dictionary preserves insertion order, making the customOrderKeepingDictimplementation unnecessary.
145-145: Updated type checks for Python 3 compatibilityGood replacement of
basestring(which doesn't exist in Python 3) with the explicit tuple(str, bytes). This ensures compatibility with Python 3 while maintaining the same behavior.Also applies to: 210-210
585-588:Details
✅ Verification successful
Modernized resource loading mechanism
Good update to use
importlib.resources.filesinstead of the olderpkg_resources.resource_filename. This is a more modern approach to resource loading that's recommended for Python 3.However, there could be a potential issue with compatibility. Let's verify that the correct module is imported:
🏁 Script executed:
#!/bin/bash # Check if importlib.resources is available in the environment python -c "import importlib.resources; print('importlib.resources is available')" || echo "importlib.resources is not available" # Check Python version to make sure it's compatible with the new API python --versionLength of output: 155
Approve modern resource loading via importlib.resources.files
The switch from
pkg_resources.resource_filenametoimportlib.resources.filesis fully compatible—your environment (Python 3.11.2) supports this API (added in Python 3.9). No further changes needed.
2-2:Details
❌ Incorrect review comment
Fix import error with future annotations
The import of
annotationsfrom__future__appears to be incorrect. This should beannotations(to support postponed evaluation of annotations) but the correct import would befrom __future__ import annotations.-from __future__ import print_function, absolute_import, annotations +from __future__ import print_function, absolute_import, annotationsLet's verify if this import is valid:
🏁 Script executed:
#!/bin/bash # Check if "annotations" is a valid __future__ import python -c "from __future__ import annotations; print('Valid import')" || echo "Invalid import"Length of output: 90
No changes needed for
from __future__ import annotationsThe import of
annotationsfrom__future__is valid (Python 3.7+), as confirmed by:python -c "from __future__ import annotations; print('Valid import')" # Output: Valid importThere is no import error, and no update is required here.
Likely an incorrect or invalid review comment.
autowrap/Code.py (2)
61-61: Updated string type check correctly for Python 3 compatibility.The change from
basestring(which doesn't exist in Python 3) to(str, bytes)is correct and maintains the same functionality of checking for any string-like type.
79-79: Updated string type check correctly for Python 3 compatibility.Proper replacement of Python 2's
basestringwith equivalent Python 3 type check using(str, bytes)tuple.autowrap/DeclResolver.py (2)
172-172: Updated type annotation to use standard dict.The type annotation has been correctly updated to use the standard
dictinstead ofOrderKeepingDict, aligning with Python 3.7+ where dictionaries maintain insertion order by default.
187-187: Changed implementation to use standard dict instead of OrderKeepingDict.Good change to utilize Python 3.7+'s built-in ordered dictionaries instead of the custom implementation. This simplifies the codebase while maintaining the same functionality.
autowrap/CodeGenerator.py (1)
1262-1262: Updated all string type checks for Python 3 compatibility.The replacement of
basestringwith(str, bytes)throughout the file ensures proper string type checking in Python 3. This change is consistent with similar updates in other modules and effectively removes Python 2 compatibility code.Also applies to: 1274-1274, 1277-1277, 1346-1346, 1359-1359, 1366-1366, 1441-1441, 1454-1454, 1462-1462, 1559-1559, 1685-1685, 1698-1698, 1705-1705, 1813-1813, 1824-1824
autowrap/ConversionProvider.py (2)
2023-2024: Approve Python 3 string conversion logic
The input conversion inStdStringUnicodeConverternow correctly checks forstr(the Python 3 Unicode type) and encodes it to UTF-8 bytes. This aligns with removing the oldunicodealias and leverages native Python 3 semantics.
2209-2211: Approve updated type check for string literals
Replacing the oldbasestringcompatibility check withisinstance(type_, (str, bytes))correctly handles both text and byte literals in Python 3.7+.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
setup.py (2)
12-12: File handling could be improved using a context managerThe current approach doesn't safely close the file after reading. Consider using a context manager instead.
-exec(open(versionfile).read()) +with open(versionfile) as f: + exec(f.read())🧰 Tools
🪛 Ruff (0.8.2)
12-12: Use a context manager for opening files
(SIM115)
35-35: Good package specification but document the exclusion rationaleThe explicit inclusion/exclusion pattern is clear, but consider adding a comment explaining why these specific packages are excluded to help future maintainers understand the reasoning.
- packages=find_namespace_packages(include=['autowrap*'], exclude=['autowrap.data_files.examples', 'autowrap.data_files.tests', 'autowrap.data_files.ez_setup']), + # Include all autowrap namespace packages except examples, tests and setup utilities + packages=find_namespace_packages(include=['autowrap*'], exclude=['autowrap.data_files.examples', 'autowrap.data_files.tests', 'autowrap.data_files.ez_setup']),.github/workflows/ci-autowrap.yaml (2)
24-29: Add stable Cython 3.0.0 across supported Python versions.
IncludingCython==3.0.0for Python 3.10, 3.11, and 3.12 ensures CI coverage for the first non-alpha release of 3.x. Verify that any Cython 3.0 API changes do not break existing code paths.Consider consolidating the matrix to reduce duplication. For example:
strategy: matrix: python-version: [3.8, 3.9, 3.10, 3.11, 3.12] cython-version: ["<=0.29.21", ">0.29.21", "3.0.0", "3.1.0"]Then use
include/excludeto skip unsupported combinations.
37-37: Upgrade actions/checkout to v4.
Bumpingactions/checkoutto v4 brings performance and security improvements. If your workflows depend on full Git history (e.g., for version tagging), consider adding:- uses: actions/checkout@v4 with: fetch-depth: 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-autowrap.yaml(1 hunks)setup.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
setup.py
12-12: Use a context manager for opening files
(SIM115)
16-16: Undefined name __version__
(F821)
🪛 GitHub Actions: build autowrap
setup.py
[error] 9-9: ModuleNotFoundError: No module named 'setuptools'. The setup.py script failed because the setuptools module is missing.
🪛 GitHub Actions: lint the codebase
setup.py
[error] 1-1: Prettier formatting check failed. File would be reformatted.
🔇 Additional comments (6)
setup.py (4)
9-9: Good update to use namespace packages!Switching from
find_packagestofind_namespace_packagesis appropriate for supporting namespace packages likeautowrap*. This aligns with modern Python packaging practices.🧰 Tools
🪛 GitHub Actions: build autowrap
[error] 9-9: ModuleNotFoundError: No module named 'setuptools'. The setup.py script failed because the setuptools module is missing.
17-19: Good metadata updatesThe maintainer information and license updates follow best practices by using a standardized SPDX identifier for the license and generic organization details.
16-16:Details
✅ Verification successful
Verify
__version__is defined in the imported fileThe static analysis tool flags an undefined
__version__variable. The code assumes this variable is defined in the executed "autowrap/version.py" file, but this should be verified.
🏁 Script executed:
#!/bin/bash # Check if __version__ is defined in version.py cat autowrap/version.py | grep -E "__version__\s*="Length of output: 79
Confirmed
__version__Definition in autowrap/version.pyVerified that
__version__is declared in autowrap/version.py as a tuple (__version__ = (0, 22, 12)), so the usage in setup.py is valid and no changes are required.🧰 Tools
🪛 Ruff (0.8.2)
16-16: Undefined name
__version__(F821)
28-29:Details
❓ Verification inconclusive
Verify Python version support removal intentionality
You've removed Python 3.7 and 3.8 support while adding 3.11 and 3.12. This is inconsistent with the PR objective which mentions Python 3.7+ compatibility for dictionary ordering. Ensure this is intentional.
🌐 Web query:
What is the minimum Python version where dictionaries maintain insertion order by default?💡 Result:
The minimum Python version where dictionaries maintain insertion order by default is Python 3.7. While CPython 3.6 preserved insertion order as an implementation detail, this behavior was formally added to the language specification in Python 3.7, making it guaranteed for all Python implementations[1][2][3][4][5][6]. Thus, you can reliably depend on insertion-order dictionaries starting with Python 3.7.
Citations:
- 1: https://softwaremaniacs.org/blog/2020/02/05/dicts-ordered/en/
- 2: https://discuss.python.org/t/can-i-rely-on-dict-insertion-order-as-stated-in-the-release-notes/87682
- 3: https://www.youtube.com/watch?v=h-DBWPjpqWY
- 4: https://community.lambdatest.com/t/how-can-i-ensure-that-a-python-dictionary-maintains-the-order-of-keys-values-as-i-declared-them/35316
- 5: https://discuss.python.org/t/sorting-a-dict-by-its-values/35272
- 6: https://www.pythonmorsels.com/dictionaries-are-ordered/
Confirm Intent for Dropping Python 3.7–3.10 Support
The PR’s objective states “Python 3.7+ compatibility” because dict insertion order is guaranteed starting in 3.7, yet the
setup.pyclassifiers now only list 3.11 and 3.12:
- File:
setup.py
Lines 28–29"Programming Language :: Python :: 3.11","Programming Language :: Python :: 3.12",
"Programming Language :: Python :: 3.11","Programming Language :: Python :: 3.12",Please confirm whether you intend to drop support for 3.7, 3.8, 3.9, and 3.10 entirely. If not, update the classifiers to include all supported 3.7+ versions.
.github/workflows/ci-autowrap.yaml (2)
19-19: Use Python 3.8 for legacy Cython compatibility.
Switching from Python 3.7 to 3.8 forCython <=0.29.21correctly addresses its incompatibility with Python 3.10+.
30-35: Test Cython 3.1.0 on the latest Python versions.
The new entries forCython==3.1.0on Python 3.10, 3.11, and 3.12 keep CI aligned with cutting-edge Cython releases. Confirm that any experimental features in Cython 3.1 don’t introduce regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/release-autowrap.yaml (1)
33-35: Consider consolidating pip install steps
You can merge the separate steps for upgrading pip, installing wheel, and installing setuptools into one to reduce setup overhead:- - name: Upgrade pip version - run: | - python -m pip install -U pip - - - name: Install wheel - run: | - python -m pip install wheel - - - name: Install setuptools - run: | - python -m pip install setuptools + - name: Install build dependencies + run: | + python -m pip install -U pip setuptools wheel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-autowrap.yaml(2 hunks).github/workflows/release-autowrap.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-autowrap.yaml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release-autowrap.yaml
21-21: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (3)
.github/workflows/release-autowrap.yaml (3)
18-18: Bump actions/checkout to v4
Upgrading from@v3to@v4aligns with the latest stable release of the checkout action and brings in performance and security improvements.
23-23: Upgrade Python version to 3.11
Moving the runner to Python 3.11 ensures compatibility with the latest language features and aligns with the overall goal of dropping older Python support.
53-59: Update GitHub Release action to v2 and rename parameter
Usingsoftprops/action-gh-release@v2and the newnameparameter (replacingrelease_name) matches the action’s updated API. This change correctly modernizes the release step.
|
Maybe check what the linter is complaining about |
with current versions of Cython we get the following error:
which appears to be due to PXDParser not defining basestring as (str, bytes). Since py2 is very dead this PR removes the compatibility blocks, replaces basestring with (str,bytes) explicitly, and for good measure gets rid of our custom OrderKeepingDict, since as of python 3.7 all the functionality is in standard dict.
Summary by CodeRabbit
Refactor
Chores
Tests
CI