-
-
Notifications
You must be signed in to change notification settings - Fork 281
Pick correct file if two files with the same name but with different extensions exist #2722
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
base: main
Are you sure you want to change the base?
Conversation
Hmm, the CI isn't too happy. Investigating... |
All tests pass on my machine now, so it should work |
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## main #2722 +/- ##
=======================================
Coverage 93.21% 93.21%
=======================================
Files 93 93
Lines 11071 11072 +1
=======================================
+ Hits 10320 10321 +1
Misses 751 751
Flags with carried forward coverage won't be shown. Click here to find out more.
π New features to boost your workflow:
|
I don't think automatic issue closing works between repos, so someone with the correct permissions needs to close it manually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix @Chasarr, do you mind adding some automated tests please ?
Done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the automated tests, it looks great. Do you mind adding a changelog entry too ? You probably need to specify that the issue you're closing come from pylint, like this:
Line 52 in df10777
Closes pylint-dev/pylint#8589 |
And if you want to have an aliase for the contributors list you can do it in https://github.com/pylint-dev/astroid/blob/main/script/.contributors_aliases.json#L4
for more information, see https://pre-commit.ci
Done! Thanks for the guidance! |
os.path.join(package, "__init__.weird_ext"), | ||
os.path.join(package, "standalone_file.weird_ext"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.path.join(package, "__init__.weird_ext"), | |
os.path.join(package, "standalone_file.weird_ext"), | |
os.path.join(package, "__init__.weird_ext"), | |
os.path.join(package, "standalone_file.weird_ext"), | |
os.path.join(package, "standalone_file.py"), |
I might have reviewed this still half asleep the first time* but shouldn't it be this instead (issue talks about 'two file with the same name but different extension') ? I suppose being able to also handle 'two file with the same extension but different name is also valuable.
* (just found and read the pylint issue to close it automatically when we merge this and it doesn't match)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There already exists an __init__.py
, so I figured I could just reuse that file for my tests too. And I also threw in a standalone file without any weird naming and without any other file sharing it's base name just because Β―\_(γ)_/Β―
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we perhaps test for __init__.py
too? I just realized there doesn't seem to be a test for that. Most likely other test would catch that error though.
* Fix bug where ``pylint code.custom_extension`` would analyze ``code.py`` or ``code.pyi`` instead if they existed. | ||
|
||
Closes pylint-dev/pylint#3631 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Fix bug where ``pylint code.custom_extension`` would analyze ``code.py`` or ``code.pyi`` instead if they existed. | |
Closes pylint-dev/pylint#3631 | |
* Fix a bug where astroid could not parse ``code.pyi`` if ``code.py`` existed. | |
Closes pylint-dev/pylint#3631 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not quite, this should not affect the behaviour of .pyi, .py or .pyw extensions. They worked as intended before my PR.
These three extensions seem to be picked according to some kind of hierarchy that can be changed with prefer_stubs = True/False.
With prefer_stubs = False:
.py, .pyw, .pyi (.pyw is only picked on Windows machines)
With prefer_stubs = True:
.pyi, .pyw, .py
So if I ran pylint mycode.pyi
it would actually check mycode.py
, if it exists. Iβm not sure why it works in this way though, but it seems to be the intended behaviour.
The bug that I fixed appeared when using a file with another file extension than these three, such as .weird_ext, where it would completely disregard my non-standard file in favor of the correctly named file. In other words,pylint mycode.weird_ext
would in reality analyze mycode.py
instead if it existed.
I donβt think I want to go too deep into what bug was fixed in the changelog, so perhaps the first explanation was sufficient? I canβt think of a better way to explain it in one sentence.
Type of Changes
Description
To replicate the error that this PR fixes, you need to create two files:
and then I run
pylint test.py2
it reports Pylint errors from test.py. (the wrong file)I believe this was due to a typo/logical error. Before this PR, I don't think the prefer_stubs argument even did anything.
Closes pylint-dev/pylint#3631