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 finding packages without an __init__.py #1333

Merged
merged 2 commits into from Jan 9, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Jan 8, 2022

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

Closes #1327

Had no time to write tests yet, so if somebody can come up with something good: please take over this PR, you're more than welcome to.
Just wanted to illustrate that this is what should be added again.

This regression was introduced in 2ee20cc

Type of Changes

Type
βœ“ πŸ› Bug fix

Related Issue

@Pierre-Sassoulas, this might warrant a 2.9.3 release.

@interifter
Copy link

If you have a pointer to how to approach testing asteroid/pylint, in general, I might be able to help. It wouldn’t be too challenging to generate a temp directory that reproduces the problem.

Otherwise, I can write a test method the way I normally would that you can adapt?

@DanielNoord
Copy link
Collaborator Author

For some reason another test broke as well. I have got some spare time now, so I'll take a look myself. Thanks anyway πŸ˜„

@DanielNoord
Copy link
Collaborator Author

While I got the test suite to fail, I can't get the added test to fail without the change (thus it doesn't test anything..).

Would need to do some further investigation into what happened before the fauly commit in this function and what is happening now.

@Pierre-Sassoulas
Copy link
Member

If you have a pointer to how to approach testing asteroid/pylint, in general, I might be able to help. It wouldn’t be too challenging to generate a temp directory that reproduces the problem.

I think recreating a temp directory that reproduce the issue is the way to go here. Thank you for your help, much appreciated !

Would need to do some further investigation

Could the problem come from the fact we removed the : os.path.expanduser(path)) ? It's the first thing that comes to mind.

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Jan 9, 2022

Test now fails with previous code and passes with new code.

The mistake was that we removed pathname = _cache_normalize_path(pathname). Whereas _canonicalize_path also normalised the path, all paths that came from path or [] where thus not normalised. This was problematic when path included "." and the normalised version of "." was not included in sys.path.
The tests also represent this scenario: we move into the tempdir and pass "." as path.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Let's release 2.9.3 with this.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 5c565b7 into pylint-dev:main Jan 9, 2022
@DanielNoord DanielNoord deleted the crash branch January 9, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants