Support Implicit Namespace Packages (PEP 420) #8153

alexey-pelykh · 2023-02-01T14:41:37Z

Fixes #7959 by introducing the concept of source roots.

Type of Changes

	Type
✓	🐛 Bug fix
✓	✨ New feature
✓	🔨 Refactoring

Description

Closes #8154

alexey-pelykh · 2023-02-01T14:42:30Z

@Pierre-Sassoulas so it is done and it would be great to get the feedback so I can address it promptly :)

codecov · 2023-02-01T14:50:15Z

Codecov Report

Merging #8153 (9428ba4) into main (4cbabc9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8153   +/-   ##
=======================================
  Coverage   95.45%   95.46%           
=======================================
  Files         177      177           
  Lines       18635    18663   +28     
=======================================
+ Hits        17788    17816   +28     
  Misses        847      847

Impacted Files	Coverage Δ
pylint/lint/base_options.py	`100.00% <ø> (ø)`
pylint/config/argument.py	`100.00% <100.00%> (ø)`
pylint/lint/__init__.py	`92.30% <100.00%> (+0.64%)`	⬆️
pylint/lint/expand_modules.py	`95.40% <100.00%> (+0.46%)`	⬆️
pylint/lint/parallel.py	`91.54% <100.00%> (+0.12%)`	⬆️
pylint/lint/pylinter.py	`95.31% <100.00%> (+<0.01%)`	⬆️
pylint/lint/utils.py	`96.55% <100.00%> (+0.55%)`	⬆️
pylint/pyreverse/main.py	`93.18% <100.00%> (+0.32%)`	⬆️
pylint/testutils/pyreverse.py	`98.07% <100.00%> (+0.07%)`	⬆️

Pierre-Sassoulas

I'm wondering if we could use the packaging information to discover the source root.

It's possible that making the user give the source root is the way to fix this once and for all. I did not review everything yet. I hope some other maintainer will have an opinion about this.

Changes to the namespace and file discovery are very frightening because there's not a lot of way to test it properly on the scale it needs to be tested to be sure it's not going to explode everywhere when we release it. So I'm very conservative when modifying this kind of code and I think we need extensive tests, maybe more (small) primers with implicit namespace package and various use cases.

tests/lint/unittest_lint.py

alexey-pelykh · 2023-02-02T05:29:22Z

@Pierre-Sassoulas thanks for your feedback and please find my comments below:

I'm wondering if we could use the packaging information to discover the source root.

A good point, absolutely. Yet that should serve as one a source of source roots information if the pylint configuration misses one and is an improvement to the current implementation. Also, such auto-magic would be a breaking change since projects would need an upgrade, as minor upgrade would bring behavior change. Probably a change for 3.0 no less.

Changes to the namespace and file discovery are very frightening because there's not a lot of way to test it properly on the scale it needs to be tested to be sure it's not going to explode everywhere when we release it. So I'm very conservative when modifying this kind of code and I think we need extensive tests, maybe more (small) primers with implicit namespace package and various use cases.

The current behavior remains unchanged so there shouldn't be a disaster once released and the test suite was expanded as well the new optional behavior.

DudeNr33 · 2023-02-03T19:17:59Z

Thank you @alexey-pelykh for this work and especially for also thinking about pyreverse 🎉

I agree with Pierre that adding some projects that use implicit namespace packages would be a good idea.

Besides that I was only wondering why it is necessary to create several new functions and deprecate the existing ones, when the now deprecated functions afterwards just delegate to the new functions.

alexey-pelykh · 2023-02-03T19:33:38Z

@DudeNr33 thanks for your feedback and please find my comments below:

I agree with Pierre that adding some projects that use implicit namespace packages would be a good idea.

Now I'm not sure I followed the idea correctly: adding projects as a test fixtures? adding where and how?
I'd be happy to do that, yet I'm not quite sure what exactly should I do :)

Besides that I was only wondering why it is necessary to create several new functions and deprecate the existing ones, when the now deprecated functions afterwards just delegate to the new functions.

Both fix_import_path and _patch_sys_path are exposed as a public API, therefore possibly are used by someone, so I couldn't add a parameter w/o default value or rename. Adding a parameter with default value while would keep the API backwards compatible, won't alert the possible users in any way that it's better to provide source root(s) explicitly to avoid pylint doing guesswork. The renaming while optional really felt right since it's not "fixing" the path, it's extending or augmenting it.

That said, the most robust solution would be to warn users that used API causes implicit guesswork via deprecation and provide an explicit way to force pylint do the guesswork via new method signatures. As a nice bonus, some naming adjustments could be be made. And in 3.0 release those deprecated methods would be removed.

DudeNr33 · 2023-02-05T08:52:10Z

Thank you for the explanation of the deprecation warnings. I agree that this is then the cleaner approach!

Now I'm not sure I followed the idea correctly: adding projects as a test fixtures? adding where and how? I'd be happy to do that, yet I'm not quite sure what exactly should I do :)

Primer tests run pylint against a selection of different repositories to check

if pylint crashes
if the changes of a pull request lead to a change in the messages generated, to help finding false positives for example

Unfortunately I have no further experience in dealing with the primer test suite, so the other maintainers can help you more with that.

alexey-pelykh · 2023-02-05T10:20:29Z

Thanks for explanation on primer tests, makes sense. Yet no open-source project that use PEP420 with 2+ depth comes to my mind really... Yet thus far nothing have changed for projects currently listed for the primer tests, right?

DudeNr33 · 2023-02-05T12:02:40Z

Thanks for explanation on primer tests, makes sense. Yet no open-source project that use PEP420 with 2+ depth comes to my mind really... Yet thus far nothing have changed for projects currently listed for the primer tests, right?

poetry-core uses implicit namespace packages, but only one level deep. Nevertheless it could be a good candidate.

Yes, for the existing primer projects all seems to be fine.

alexey-pelykh · 2023-02-05T14:56:01Z

poetry-core uses implicit namespace packages, but only one level deep.

It is an ideal one as is appears, exactly what I was looking for. So I'll add it :)

alexey-pelykh · 2023-02-06T08:44:52Z

@Pierre-Sassoulas any insights what was this about? https://github.com/PyCQA/pylint/blob/main/pylint/testutils/_primer/primer_run_command.py#L78

Also, it feels like #8193 needs to go into main before this one to properly capture unchanged poetry-core priming?

Pierre-Sassoulas · 2023-02-06T09:44:34Z

@Pierre-Sassoulas any insights what was this about? https://github.com/PyCQA/pylint/blob/main/pylint/testutils/_primer/primer_run_command.py#L78

duplicate code is really slow, intrinsically solving a hard problem that scale pretty badly on big code base and the code almost never change so we removed it. For cyclic import I don't remember but maybe it's also slowness, maybe also that the result are not the same each run (?)

alexey-pelykh · 2023-02-06T12:41:35Z

@Pierre-Sassoulas please rerun https://github.com/PyCQA/pylint/actions/runs/4102946645/jobs/7076537678 otherwise nothing will happen

Pierre-Sassoulas · 2023-02-06T13:23:11Z

Trying to fix this in #8200

alexey-pelykh · 2023-02-07T12:53:00Z

Well, I guess all is in check now?

DudeNr33

I only have a few wording suggestions, but they are no blocker imo.
However I think at least one other maintainer should additionally approve before merging.

doc/user_guide/usage/run.rst

doc/whatsnew/fragments/8154.feature

Co-authored-by: Andreas Finkler <[email protected]>

alexey-pelykh · 2023-02-07T17:24:33Z

No objections on proposed changes :)

github-actions · 2023-02-07T18:14:57Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 9428ba4

Pierre-Sassoulas

Thank you for all the work, clearly a lot of care went into this. I don't see anything outrageous and there are tests that I think are reasonable. Let's wait a little for more reviews. Then we could release a 2.17 beta version to make sure there's no obvious issues before really releasing in the wild :)

Pierre-Sassoulas · 2023-02-07T18:16:27Z

pylint/lint/utils.py

+    # TODO: Remove deprecated function
+    warnings.warn(
+        "_patch_sys_path has been deprecated because it relies on auto-magic package path "
+        "discovery which is implemented by get_python_path that is deprecated. "


We could announce a removal in 3.0, maybe 4.0 seeing that 3.0 is nearing.

DudeNr33

Looks good to me. 😊

alexey-pelykh requested a review from DudeNr33 as a code owner February 1, 2023 14:41

alexey-pelykh force-pushed the feature/source-roots branch 3 times, most recently from 49ca119 to ae88477 Compare February 1, 2023 15:06

This comment has been minimized.

Sign in to view

alexey-pelykh force-pushed the feature/source-roots branch 2 times, most recently from e37d01d to 6844c72 Compare February 1, 2023 16:46

Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Feb 1, 2023

Pierre-Sassoulas added this to the 2.17.0 milestone Feb 1, 2023

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Feb 1, 2023

View reviewed changes

jacobtylerwalls reviewed Feb 2, 2023

View reviewed changes

tests/lint/unittest_lint.py Show resolved Hide resolved

Pierre-Sassoulas self-requested a review February 3, 2023 14:51

alexey-pelykh force-pushed the feature/source-roots branch from 981ece2 to 3403b8a Compare February 5, 2023 14:55

alexey-pelykh force-pushed the feature/source-roots branch from 3403b8a to 3b367f5 Compare February 6, 2023 08:43

alexey-pelykh mentioned this pull request Feb 6, 2023

Add poetry-core as a repository to prime #8193

Merged

alexey-pelykh force-pushed the feature/source-roots branch 3 times, most recently from eb80074 to 366c597 Compare February 6, 2023 12:16

alexey-pelykh force-pushed the feature/source-roots branch from 366c597 to 34bbef6 Compare February 6, 2023 15:33

This comment has been minimized.

Sign in to view

alexey-pelykh force-pushed the feature/source-roots branch from 34bbef6 to be0f56d Compare February 7, 2023 08:05

This comment has been minimized.

Sign in to view

Support Implicit Namespace Packages (PEP 420)

b8c5c56

alexey-pelykh force-pushed the feature/source-roots branch from be0f56d to b8c5c56 Compare February 7, 2023 11:48

This comment has been minimized.

Sign in to view

DudeNr33 previously approved these changes Feb 7, 2023

View reviewed changes

doc/user_guide/usage/run.rst Outdated Show resolved Hide resolved

doc/whatsnew/fragments/8154.feature Outdated Show resolved Hide resolved

Update doc/user_guide/usage/run.rst

0e3986b

Co-authored-by: Andreas Finkler <[email protected]>

alexey-pelykh dismissed DudeNr33’s stale review via 0e3986b February 7, 2023 17:23

Update doc/whatsnew/fragments/8154.feature

9428ba4

Co-authored-by: Andreas Finkler <[email protected]>

Pierre-Sassoulas approved these changes Feb 7, 2023

View reviewed changes

DudeNr33 approved these changes Feb 9, 2023

View reviewed changes

Pierre-Sassoulas added Enhancement ✨ Improvement to a component and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Feb 9, 2023

Pierre-Sassoulas merged commit 71b6325 into pylint-dev:main Feb 9, 2023

alexey-pelykh deleted the feature/source-roots branch February 9, 2023 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Implicit Namespace Packages (PEP 420) #8153

Support Implicit Namespace Packages (PEP 420) #8153

alexey-pelykh commented Feb 1, 2023 •

edited

Loading

alexey-pelykh commented Feb 1, 2023

codecov bot commented Feb 1, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

Pierre-Sassoulas left a comment

alexey-pelykh commented Feb 2, 2023

DudeNr33 commented Feb 3, 2023

alexey-pelykh commented Feb 3, 2023

DudeNr33 commented Feb 5, 2023

alexey-pelykh commented Feb 5, 2023 •

edited

Loading

DudeNr33 commented Feb 5, 2023

alexey-pelykh commented Feb 5, 2023

alexey-pelykh commented Feb 6, 2023

Pierre-Sassoulas commented Feb 6, 2023

alexey-pelykh commented Feb 6, 2023

Pierre-Sassoulas commented Feb 6, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexey-pelykh commented Feb 7, 2023

DudeNr33 left a comment

alexey-pelykh commented Feb 7, 2023

github-actions bot commented Feb 7, 2023

Pierre-Sassoulas left a comment

Pierre-Sassoulas Feb 7, 2023

DudeNr33 left a comment

Support Implicit Namespace Packages (PEP 420) #8153

Support Implicit Namespace Packages (PEP 420) #8153

Conversation

alexey-pelykh commented Feb 1, 2023 • edited Loading

Type of Changes

Description

alexey-pelykh commented Feb 1, 2023

codecov bot commented Feb 1, 2023 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

alexey-pelykh commented Feb 2, 2023

DudeNr33 commented Feb 3, 2023

alexey-pelykh commented Feb 3, 2023

DudeNr33 commented Feb 5, 2023

alexey-pelykh commented Feb 5, 2023 • edited Loading

DudeNr33 commented Feb 5, 2023

alexey-pelykh commented Feb 5, 2023

alexey-pelykh commented Feb 6, 2023

Pierre-Sassoulas commented Feb 6, 2023

alexey-pelykh commented Feb 6, 2023

Pierre-Sassoulas commented Feb 6, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexey-pelykh commented Feb 7, 2023

DudeNr33 left a comment

Choose a reason for hiding this comment

alexey-pelykh commented Feb 7, 2023

github-actions bot commented Feb 7, 2023

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas Feb 7, 2023

Choose a reason for hiding this comment

DudeNr33 left a comment

Choose a reason for hiding this comment

alexey-pelykh commented Feb 1, 2023 •

edited

Loading

codecov bot commented Feb 1, 2023 •

edited

Loading

alexey-pelykh commented Feb 5, 2023 •

edited

Loading