Skip to content

Removed unused detection of site-packages directory #5874

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 8, 2022
Merged

Removed unused detection of site-packages directory #5874

merged 4 commits into from
Mar 8, 2022

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Mar 7, 2022 •
edited
Loading

Type of Changes

Type
✓ 🔨 Refactoring

Description

Remove use of deprecated module distutils by relying on astroid's determination of the location of site packages. It seems error-prone to have two places for this logic.

EDIT: remove unused functionality for determining the location of site-packages (unused since 9812c5a).

@jacobtylerwalls
Rely on astroid for location of site-packages
d05ee30 
Remove use of deprecated module `distutils`
DanielNoord
DanielNoord approved these changes Mar 7, 2022
View reviewed changes
@jacobtylerwalls @DanielNoord
Add more typing
dd459c8 
Co-authored-by: Daniël van Noord <[email protected]>
@jacobtylerwalls jacobtylerwalls changed the title Rely on astroid for location of site-packages Rely on astroid for location of stdlib packages Mar 7, 2022
@jacobtylerwalls jacobtylerwalls mentioned this pull request Mar 7, 2022
Merged
@coveralls
Copy link

coveralls commented Mar 7, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1948863365

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 93.998%
Totals Coverage Status
Change from base Build 1948147488: -0.007%
Covered Lines: 14926
Relevant Lines: 15879
💛 - Coveralls

@jacobtylerwalls
Copy link
Member Author

I agree it's wise to put this in 2.14, but I suggest doing it right after 2.13 so it doesn't block contributors who have python 3.10 interpreters for pre-commit.

(Pylint's pre-commit hook doesn't specify a version, correct? 3.8 is running on CI. I had to --no-verify my changes on #5864 when removing the refactor.)

@DanielNoord
Copy link
Collaborator

(Pylint's pre-commit hook doesn't specify a version, correct? 3.8 is running on CI. I had to --no-verify my changes on #5864 when removing the refactor.)

On mobile, so I can't check. But: does deprecated-module do anything with the python-version setting? If your python-version is set to 3.6, should we emit deprecated-module warnings for distutils?

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Mar 7, 2022 •
edited
Loading

On mobile, so I can't check. But: does deprecated-module do anything with the python-version setting? If your python-version is set to 3.6, should we emit deprecated-module warnings for distutils?

Good question. The deprecated checker uses a map keyed on python versions so that warnings won't be emitted for earlier versions, e.g. distutils on python 3.9.

I suppose we should just specify python3.8 in the pre-commit config?

language_version: python3.8

@cdce8p
Copy link
Member

cdce8p commented Mar 7, 2022

On mobile, so I can't check. But: does deprecated-module do anything with the python-version setting? If your python-version is set to 3.6, should we emit deprecated-module warnings for distutils?

That was my question too. However it seems, the checker doesn't respect the py-version setting currently. Don't know though if that has been intentional or simply because py-version was added later.

It's definitely something worth exploring further IMO.

I suppose we should just specify python3.8 in the pre-commit config?

language_version: python3.8

pylint is defined as a system hook, since we need the custom environment with all dependencies. So that wouldn't work.

cdce8p
cdce8p requested changes Mar 8, 2022
View reviewed changes
pylint/checkers/imports.py Outdated
Comment on lines 442 to 446
self._site_packages = self._compute_site_packages()

@staticmethod
def _compute_site_packages():
def _normalized_path(path):
def _compute_site_packages() -> Set[str]:
def _normalized_path(path: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Another question: Where is it still used? I wanted to check if STD_LIB_DIRS is actual the correct replacement but couldn't find any code that uses self._site_packges.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it looks unused. pylint used to rely on EXT_LIB_DIRS, until these special cases were added in 6e70374. Then the use of _site_packages was removed in 9812c5a. I think this can just be removed without a replacement.

pylint/checkers/imports.py Outdated
)
paths.add(libpython)
return paths
return {_normalized_path(path) for path in astroid.modutils.STD_LIB_DIRS}
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
return {_normalized_path(path) for path in astroid.modutils.STD_LIB_DIRS}
return {_normalized_path(path) for path in astroid.modutils.EXT_LIB_DIRS}

If these should be the paths to the site-packages dirs, it should be EXT_LIB_DIRS instead.
See pylint-dev/astroid#1322

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, bad mistake. That was the PR I was looking at yesterday when I started down this path, and then I mistook the recent astroid changes for being relevant. 🤕

@jacobtylerwalls jacobtylerwalls changed the title Rely on astroid for location of stdlib packages Removed unused site-packages detection Mar 8, 2022
@jacobtylerwalls jacobtylerwalls changed the title Removed unused site-packages detection Removed unused detection of site-packages directory Mar 8, 2022
@jacobtylerwalls jacobtylerwalls mentioned this pull request Mar 8, 2022
Merged
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Mar 8, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Mar 8, 2022
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice catch that it was unused ! In the end if we're just removing I think it can go in 2.13 ? (Btw we just reached 94% coverage, this kind of cleanup helps a lot to make the code base easier to maintain).

@cdce8p
Copy link
Member

cdce8p commented Mar 8, 2022

Thanks @jacobtylerwalls 🐬

@cdce8p cdce8p merged commit 6c0ba13 into pylint-dev:main Mar 8, 2022
@jacobtylerwalls jacobtylerwalls deleted the remove-distutils branch March 8, 2022 12:41
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Mar 9, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@DanielNoord DanielNoord DanielNoord approved these changes

@cdce8p cdce8p cdce8p approved these changes

Assignees
No one assigned
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jacobtylerwalls @coveralls @DanielNoord @cdce8p @Pierre-Sassoulas