-
Notifications
You must be signed in to change notification settings - Fork 23
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
[CG-10889] fix: Nested package #611
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files |
tests/unit/codegen/sdk/python/import_resolution/test_import_resolution.py
Show resolved
Hide resolved
Overall, this is a solid improvement to the import resolution system. The changes are focused and the test coverage demonstrates the functionality well. A few suggestions for consideration:
The changes look good to merge after addressing the documentation suggestion. The test coverage suggestion would be a nice-to-have but isn't blocking. |
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.
TLDR
Hmm I feel like this entails us supporting reading pyproject.toml the way we do with tsconfig.json?
IMO #601 is a better approach for that than us hacking it together?
Full comment
For context, python supports resolving imports the following way:
- You get a list of paths that comprise PYTHONPATH/sys.path. This generally includes the project root/cwd (as "")
- For each path, we do to resolve this import in a given path
We have some implementations for 1:
- base_path (our current method to override this manually)
- src/test detection (which I believe is standard for packages?)
For this case, we can
- Add hacks for individual cases like this PR
- Support reading sys.path/PYTHONPATH (Allow custom overrites for package resolving and optional sys.path support #601, my preference)
- Set the base_path as needed (current solution)
Thoughts?
addressed in #601 |
Motivation
Content
Testing
Please check the following before marking your PR as ready for review