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

Partially remove dependency on imp. #686

Closed
wants to merge 1 commit into from
Closed

Partially remove dependency on imp. #686

wants to merge 1 commit into from

Conversation

degustaf
Copy link
Contributor

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

This partially removes the dependency on the deprecated imp module.

Type of Changes

Type
🐛 Bug fix

Related Issue

This is related to issues #594 and #681

@degustaf
Copy link
Contributor Author

Note that removing the final dependency on imp requires making a decision. It is located in modutils.load_module_from_modpath and is a pair of calls to imp.find_module and imp.load_module. The official documentation recommends replacing these with importlib.import_module. For this use case it does not allow for ignoring sys.modules, which the API allows.

I see 2 options for how to solve this:

  1. Change the API to remove the option to ignore sys.module. This would affect the functions modutils.load_module_from_modpath, modutils.load_module_from_name, and modutils.load_module_from_file. This option does not appear to be used anywhere in astroid or pylint, except for 1 unittest.

  2. Copy the code from imp.py so that we maintain the functionality by maintaining the code.

Copy link
Contributor

@gyermolenko gyermolenko left a comment

Choose a reason for hiding this comment

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

ImpFinder is """A finder based on the imp module.""" so importlib-related stuff should probably go into separate class.

module_type=_imp_type_to_module_type(mp_desc[2]),
else:
try:
loader = importlib.util.find_spec(modname)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious. The function returns ModuleSpec object, why did you name it loader?

@gyermolenko
Copy link
Contributor

Don't know if it would help, but here is ref to similar improvement in pytest-dev/pytest#5468

@PCManticore
Copy link
Contributor

This is on my todo list for reviewing. Unfortunately might take me a bit as I'll be in vacation for the next two weeks.

Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

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

Hey @degustaf Thank you for the PR and sorry for this extreme delay! This all looks good to me, and option 1 sounds the best, since it does not seem that we rely on use_sys neither here, nor in pylint, so I'd say we should go with that.

pass
submodule_path = sys.path

suffixes = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to build this list globally instead of building it every time this function is called?

@PCManticore
Copy link
Contributor

@degustaf Can you rebase your PR when you get a chance? I'd like to pull this in. Thank you!

@degustaf
Copy link
Contributor Author

@PCManticore I should be able to do that tonight.

@stonebig
Copy link

stonebig commented Apr 9, 2020

any hope on this ? I see some packages have made the transition like ansible/ansible#54883

bmwiedemann added a commit to bmwiedemann/openSUSE that referenced this pull request Jun 26, 2020
https://build.opensuse.org/request/show/816653
by user mcepl + dimstar_suse
- Add part_rm_dep_imp.patch to replace missing `imp` package
  (gh#pylint-dev/astroid#686).
@pkolbus pkolbus mentioned this pull request Jul 28, 2020
@pkolbus pkolbus mentioned this pull request Nov 22, 2020
2 tasks
@hippo91
Copy link
Contributor

hippo91 commented Dec 13, 2020

I close this issue because #857 has been merged.

@hippo91 hippo91 closed this Dec 13, 2020
@pkolbus pkolbus mentioned this pull request May 22, 2021
2 tasks
@degustaf degustaf deleted the remove_imp branch June 12, 2023 12:41
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.

5 participants