Skip to content
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

Relative imports should work even if they are not within the project #1684

Merged
merged 6 commits into from Dec 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 36 additions & 1 deletion jedi/inference/imports.py
Expand Up @@ -245,7 +245,42 @@ def _sys_path_with_modifications(self, is_completion):
)

def follow(self):
if not self.import_path or not self._infer_possible:
if not self.import_path:
if self._fixed_sys_path:
# This is a bit of a special case, that maybe should be
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description here of why the code is needed/is special feels out of step with the test which validates the behaviour. Perhaps I'm missing something, but is there more to this code than just the niche case described here?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's simply that.

It's just a special case, because it breaks the basic idea of something being on the sys path. Do you need further explanations?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry. I think if this code were here just for the special case, which this comment suggests is to do with the same name appearing at several levels in the hierarchy, then on reading this comment alone I would assume that it would only be activated by that special case and that the test to validate it would exercise that case. However the test doesn't actually seem to hit that case (the directory names it chooses differ at each level), yet this code path is taken.

Clearly the test that is present is a useful test and this codepath is useful in making it work, so perhaps this comment could be tweaked to clarify that this codepath is here for all cases of namespace packages, rather than just for the case of matching names?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my comment just wasn't good enough. Because having the same names is just one case. Will try to improve :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a bit to the comment.

Once I merge this, I will probably make a release.

Also sorry about the delay, but had to await my firstborn :)

# revisited. If the project path is wrong or the user uses
# relative imports the wrong way, we might end up here, where
# the `fixed_sys_path == project.path` in that case we kind of
# use the project.path.parent directory as our path. This is
# usually not a problem, except if imports in other places are
# using the same names. Example:
#
# foo/ < #1
# - setup.py
# - foo/ < #2
# - __init__.py
# - foo.py < #3
#
# If the top foo is our project folder and somebody uses
# `from . import foo` in `setup.py`, it will resolve to foo #2,
# which means that the import for foo.foo is cached as
# `__init__.py` (#2) and not as `foo.py` (#3). This is usually
# not an issue, because this case is probably pretty rare, but
# might be an issue for some people.
#
# However for most normal cases where we work with different
# file names, this code path hits where we basically change the
# project path to an ancestor of project path.
from jedi.inference.value.namespace import ImplicitNamespaceValue
import_path = (os.path.basename(self._fixed_sys_path[0]),)
ns = ImplicitNamespaceValue(
self._inference_state,
string_names=import_path,
paths=self._fixed_sys_path,
)
return ValueSet({ns})
return NO_VALUES
if not self._infer_possible:
return NO_VALUES

# Check caches first
Expand Down
6 changes: 4 additions & 2 deletions test/test_api/test_usages.py
@@ -1,9 +1,11 @@
import pytest

from ..helpers import test_dir


def test_import_references(Script):
davidhalter marked this conversation as resolved.
Show resolved Hide resolved
s = Script("from .. import foo", path="foo.py")
assert [usage.line for usage in s.get_references(line=1, column=18)] == [1]
davidhalter marked this conversation as resolved.
Show resolved Hide resolved
s = Script("from .. import foo", path=test_dir.joinpath("foo.py"))
assert [usage.line for usage in s.get_references()] == [1]


def test_exclude_builtin_modules(Script):
Expand Down
21 changes: 21 additions & 0 deletions test/test_inference/test_imports.py
Expand Up @@ -8,6 +8,7 @@

import pytest

import jedi
from jedi.file_io import FileIO
from jedi.inference import compiled
from jedi.inference import imports
Expand Down Expand Up @@ -470,6 +471,26 @@ def test_relative_import_star(Script):
assert script.complete(3, len("furl.c"))


@pytest.mark.parametrize('with_init', [False, True])
def test_relative_imports_without_path_and_setup_py(
Script, inference_state, environment, tmpdir, with_init):
# Contrary to other tests here we create a temporary folder that is not
# part of a folder with a setup.py that signifies
tmpdir.join('file1.py').write('do_foo = 1')
other_path = tmpdir.join('other_files')
other_path.join('file2.py').write('def do_nothing():\n pass', ensure=True)
if with_init:
other_path.join('__init__.py').write('')

for name, code in [('file2', 'from . import file2'),
('file1', 'from .. import file1')]:
for func in (jedi.Script.goto, jedi.Script.infer):
n, = func(Script(code, path=other_path.join('test1.py').strpath))
assert n.name == name
assert n.type == 'module'
assert n.line == 1


def test_import_recursion(Script):
path = get_example_dir('import-recursion', "cq_example.py")
for c in Script(path=path).complete(3, 3):
Expand Down