Fix/isort python39 compatibility#142
Conversation
- Add conditional isort installation in [project.optional-dependencies] - Python 3.10+: isort >=7.0.0 - Python 3.9: isort >=5.12.0, <7.0 - Remove hardcoded isort from [tool.poetry.dev-dependencies] - Update requirements.txt with documentation comment - Maintains backwards compatibility with Python 3.9 while allowing Python 3.10+ users to use isort v7 Fixes Python 3.9 CI/CD compatibility issue with isort v7 upgrade. This PR builds on top of PR kinde-oss#126 (isort v7 upgrade) and adds Python 3.9 compatibility.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced the single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
dtoxvanilla1991
left a comment
dtoxvanilla1991
left a comment
There was a problem hiding this comment.
Line 34 currently pins isort>=5.12.0 unconditionally in requirements.txt, so any workflow that still relies on pip install -r requirements.txt (many CI jobs do) will keep pulling the 5.x series even on Python 3.10+. That contradicts the PR’s stated goal of allowing 3.10+ to use isort 7.
If you really expect everyone—including automation—to switch to pip install -e ".[dev]", then yes, that isort line should be removed so there isn’t a conflicting requirement. But before dropping it, confirm that no CI/CD paths still consume requirements.txt; otherwise you’ll lose isort entirely there. A safer alternative is to keep entries in requirements.txt but mirror the conditional markers already used in pyproject.toml,
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@requirements.txt`:
- Around line 31-35: The pyproject.toml dev-dependencies currently pins isort =
"^7.0.0" under [tool.poetry.dev-dependencies], which conflicts with the
conditional isort constraints in requirements.txt and
[project.optional-dependencies]. Update pyproject.toml to either remove the
unconditional isort entry or replace it with a Python-version-conditional entry
that mirrors requirements.txt (e.g., provide an isort fallback for
python_version < "3.10"), and add an explanatory comment similar to the existing
pylint pattern so Poetry users on 3.9 resolve the compatible isort version
instead of 7.x.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e018d300-8655-4ace-afe4-7e52439b1487
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (1)
requirements.txt
There was a problem hiding this comment.
🧹 Nitpick comments (2)
testv2/testv2_framework/test_flask_framework.py (1)
7-7: Prefer standardimport pytestover__import__("pytest").The
__import__()function is typically used for dynamic imports where the module name isn't known at write time. Here, a standardimport pyteststatement is clearer and more idiomatic.♻️ Suggested simplification
-pytest = __import__("pytest") +import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testv2/testv2_framework/test_flask_framework.py` at line 7, Replace the dynamic import expression that assigns pytest via __import__("pytest") with a standard top-level import statement; locate the usage of the pytest symbol in test_flask_framework.py and change the line "pytest = __import__(\"pytest\")" to the idiomatic "import pytest" so the module is imported normally and the pytest name is available throughout the file.testv2/testv2_framework/test_fastapi_framework.py (1)
4-4: Prefer standardimport pytestover__import__("pytest").Same as in the Flask test file—a standard
import pytestis clearer and more idiomatic than using__import__().♻️ Suggested simplification
-pytest = __import__("pytest") +import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testv2/testv2_framework/test_fastapi_framework.py` at line 4, Replace the dynamic import usage pytest = __import__("pytest") with the standard import statement by changing the module import to a normal top-level import (use import pytest) so tests use the idiomatic pytest symbol; update any references relying on the current assignment if needed to use the imported pytest module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@testv2/testv2_framework/test_fastapi_framework.py`:
- Line 4: Replace the dynamic import usage pytest = __import__("pytest") with
the standard import statement by changing the module import to a normal
top-level import (use import pytest) so tests use the idiomatic pytest symbol;
update any references relying on the current assignment if needed to use the
imported pytest module.
In `@testv2/testv2_framework/test_flask_framework.py`:
- Line 7: Replace the dynamic import expression that assigns pytest via
__import__("pytest") with a standard top-level import statement; locate the
usage of the pytest symbol in test_flask_framework.py and change the line
"pytest = __import__(\"pytest\")" to the idiomatic "import pytest" so the module
is imported normally and the pytest name is available throughout the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80e80aff-70d7-4e9b-9c4e-3b36e10093bc
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (2)
testv2/testv2_framework/test_fastapi_framework.pytestv2/testv2_framework/test_flask_framework.py
3 participants
fix: make isort conditional to maintain Python 3.9 compatibility
Python 3.10+ users to use isort v7
Fixes Python 3.9 CI/CD compatibility issue with isort v7 upgrade.
This PR builds on top of PR #126 (isort v7 upgrade).
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.