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 930 pyreverse regression #984

Merged
merged 9 commits into from
May 24, 2021

Conversation

DudeNr33
Copy link
Contributor

@DudeNr33 DudeNr33 commented May 1, 2021

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

PR #857 introduced a regression for modutils.is_relative().
Calling this function failed if the inspected import statement was inside a module of a package at least two levels deep (e.g. astroid.interpreter._import.spec, see the added unit tests).

Inspecting the old implementation of is_relative which relied on imp and the implementation of imp.find_module, this only checked of the given modname (or better to say the first part after splitting it on ".") was a module or package inside the same package as the from_file itself.

The new implementation seemed to do a lot more, and I could not really understand what exactly.

Anyways, importlib.machinery.PathFinder().find_spec seems to be a better replacement for imp.find_module, makes the implementation simpler, passes all the tests and pyreverse is working again.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #930
Closes pylint-dev/pylint#4186

@DudeNr33 DudeNr33 marked this pull request as ready for review May 1, 2021 16:15
@DudeNr33
Copy link
Contributor Author

DudeNr33 commented May 7, 2021

@Pierre-Sassoulas or @cdce8p could you please approve the workflow run?
I fixed the pre-commit checks that were failing on the previous run.

@hippo91 hippo91 self-assigned this May 14, 2021
Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

@DudeNr33 thanks for this clear, concise and tested MR!
Maybe @cdce8p could also give his opinion on this.
Before merging it to master i would also like to have @pkolbus (author of #857) opinion on the use of PathFinder class.

submodule_spec = importlib.util.find_spec(
name + "." + modname.split(".")[0], parent_spec.submodule_search_locations
modspec = importlib.machinery.PathFinder().find_spec(
modname.split(".")[0], [from_file]
Copy link
Contributor

Choose a reason for hiding this comment

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

From what i can read in the PathFinder doc, it seems to do the job. However i would like to have @pkolbus opinion on this. Maybe he could tell us if did not use the PathFInder for a specific reason.
Instead of split you can use partition. Performance is a bit better as it doesn't split multiple times the string but just one. Moreover, as the PathFinder doc say, there is no need to instantiate this class as all its methods are @classmethod.
We thus could directly write:

return importlib.machinery.PathFinder.find_spec(modname.partition(".")[0], [from_file])

Copy link
Contributor

Choose a reason for hiding this comment

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

This part of #857 wasn’t my work actually, it came from @degustaf in #686. I didn’t run into any errors with it, so didn’t evaluate alternatives. @degustaf, can you provide insight here?

Copy link
Member

Choose a reason for hiding this comment

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

It should probably be:

    return bool(
        importlib.machinery.PathFinder.find_spec(
            modname.split(".", maxsplit=1)[0], [from_file]
        )
    )

Copy link
Contributor

Choose a reason for hiding this comment

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

This part of #857 wasn’t my work actually, it came from @degustaf in #686. I didn’t run into any errors with it, so didn’t evaluate alternatives. @degustaf, can you provide insight here?

Thanks @pkolbus !

@cdce8p cdce8p added this to the 2.5.7 milestone May 23, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

I noticed that astroid/modutils.py seems to be missing some imports

import importlib
import importlib.machinery

submodule_spec = importlib.util.find_spec(
name + "." + modname.split(".")[0], parent_spec.submodule_search_locations
modspec = importlib.machinery.PathFinder().find_spec(
modname.split(".")[0], [from_file]
Copy link
Member

Choose a reason for hiding this comment

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

It should probably be:

    return bool(
        importlib.machinery.PathFinder.find_spec(
            modname.split(".", maxsplit=1)[0], [from_file]
        )
    )

@cdce8p cdce8p added the pylint-tested PRs that don't cause major regressions with pylint label May 23, 2021
@DudeNr33
Copy link
Contributor Author

I noticed that astroid/modutils.py seems to be missing some imports

import importlib
import importlib.machinery

Added the imports and removed the instantiation of the PathFinder class.
Interesting that it already worked without the imports - and while PyCharm actually complained about the imports, VS Code did not.

submodule_spec = importlib.util.find_spec(
name + "." + modname.split(".")[0], parent_spec.submodule_search_locations
return bool(
importlib.machinery.PathFinder.find_spec(modname.partition(".")[0], [from_file])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
importlib.machinery.PathFinder.find_spec(modname.partition(".")[0], [from_file])
importlib.machinery.PathFinder.find_spec(modname.split(".", maxsplit=1)[0], [from_file])

We started to use split with maxsplit=1 instead for pylint. Might be an idea to use here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it, thanks for the suggestion.

ChangeLog Outdated Show resolved Hide resolved
@cdce8p cdce8p merged commit f6ab109 into pylint-dev:master May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pylint-tested PRs that don't cause major regressions with pylint Waiting on author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pyreverse regression after #857 (astroid 2.5) pyreverse fails with ModuleNotFoundError
5 participants