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

fix(import): support for nested namespace packages #1805

Merged

Conversation

kirat-singh
Copy link
Contributor

If multiple directories in sys.path provide a nested namespace package, then jedi would only visit the first directory which
contained the package. Fix this by saving the remaining path list in the ImplicitNamespaceValue and add a test for it.

Added test namespace packages to demonstrate the failure:

namespace1/ namespace1/pkg1 namespace1/pkg1/pkg2 namespace1/pkg1/pkg2/mod1.py namespace2 namespace2/pkg1 namespace2/pkg1/pkg2 namespace2/pkg1/pkg2/mod2.py

If both namespace1 and namespace2 are added to sys path, then jedi sees only mod1 without the fix. After the fix it correctly sees both mod1 and mod2.

If multiple directories in sys.path provide a nested namespace
package, then jedi would only visit the first directory which
contained the package.  Fix this by saving the remaining path list in
the ImplicitNamespaceValue and add a test for it.
@kirat-singh
Copy link
Contributor Author

Whoops this is a duplicate of #1784 I'm happy to withdraw this if the other one is approved.

@codecov-commenter
Copy link

Codecov Report

Merging #1805 (53e8370) into master (65bc1c1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1805   +/-   ##
=======================================
  Coverage   94.50%   94.50%           
=======================================
  Files          80       80           
  Lines       11817    11817           
=======================================
  Hits        11168    11168           
  Misses        649      649           
Impacted Files Coverage Δ
jedi/inference/imports.py 98.62% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65bc1c1...53e8370. Read the comment docs.

@davidhalter
Copy link
Owner

I guess the isinstance there is kind of weird and does not make sense. I guess just change it to

        file_io_or_ns, is_pkg = inference_state.compiled_subprocess.get_module_info(
            string=import_names[-1],
            path=paths,
            full_name=module_name,
            is_global_search=False,
        )
        if is_pkg is None:
            return NO_VALUES

The for loop here is removed and not needed anymore. I do not know why it was there in the first place. Probably some refactorings that made it unnecessary.

@davidhalter
Copy link
Owner

#1784 is a duplicate, but it changes some other things as well, so we might as well merge both and resolve the merge conflict :)

The earlier fix will probably just win ;-).

@kirat-singh
Copy link
Contributor Author

thanks for looking, and agree should have simplified. changed it as you suggested.

@davidhalter davidhalter merged commit bb5bed4 into davidhalter:master Oct 9, 2021
@davidhalter
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants