-
-
Notifications
You must be signed in to change notification settings - Fork 593
feat(gazelle) For package mode, resolve dependencies when imports are relative to the package path #2865
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
base: main
Are you sure you want to change the base?
Conversation
37c9386
to
8914db1
Compare
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.
LGTM. @aignas Can you take a look?
Don't have time right now to look into gazelle PRs at a great detail, so I'll defer this one to @dougthor42, Thanks @linzhp, for the initial review! |
@yushan26 can you take a look at the discussion in #2203? There are some edge case issues that I'd like to know if you've addressed. It looks like you're focusing on the "importing a class/function identifier" case. Does this also support importing relative modules? from ..my_library import some_function # looks like you've got this covered
from ...my_library.foo import some_function # this too
from .library import other_module
from .. import some_module
from .. import some_function # eg the function is exposed in ../__init__.py Also, if we add support for relative imports, we need to make sure that it works when |
To clarify, relative imports are already supported before this PR (see this test), but it's partially broken in package mode (on importing another package) and not supported in file mode. This PR fixes the package mode. I am leaning towards fixing the file mode in a separate PR, unless you feel strongly about fixing both of them in one PR. |
24ace24
to
7d1f434
Compare
Yea good point. Updated the test case to support the scenario and updated the function. |
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.
I am leaning towards fixing the file mode in a separate PR
SGTM.
Other requests:
- Please update the PR title to follow the guidelines.
- Please update the changelog.
gazelle/python/testdata/relative_imports_package_mode/README.md
Outdated
Show resolved
Hide resolved
...lle/python/testdata/relative_imports_package_mode/package1/subpackage1/subpackage2/script.py
Outdated
Show resolved
Hide resolved
...lle/python/testdata/relative_imports_package_mode/package1/subpackage1/subpackage2/script.py
Outdated
Show resolved
Hide resolved
gazelle/python/testdata/relative_imports_project_mode/README.md
Outdated
Show resolved
Hide resolved
@dougthor42 Thanks for the review. I also included the directive |
When
# gazelle:python_generation_mode package
is enabled, relative imports are currently not being added to thedeps
field of the generated target.For example, given the following Python code:
The expected py_library rule should include a dependency on the local library package:
However, the actual generated rule is missing the deps entry:
This change updates file_parser.go to ensure that relative imports (those starting with a .) are parsed and preserved. In
Resolve()
, logic is added to correctly interpret relative paths:A single dot (.) refers to the current package.
Multiple dots (.., ..., etc.) traverse up parent directories.
The relative import is resolved against the current label.Pkg path that imports the module and converted into an path relative to the root before dependency resolution.
As a result, dependencies for relative imports are now correctly added to the deps field in package generation mode.
Added a directive
# gazelle:experimental_allow_relative_imports true
to allow this feature to be opt in.