Fix for issue #10969 - Pylint skipping similarly named project direct…#10970
Fix for issue #10969 - Pylint skipping similarly named project direct…#10970cinnion wants to merge 1 commit intopylint-dev:mainfrom
Conversation
…ect directory. - Because _discover_files() will falsely ignore a with a name like `applications_api` after walking across one named `applications`, add the os directory separator to the end of what is a known directory (because of the fact that it has a file named `__init__.py` in it).
DanielNoord
left a comment
There was a problem hiding this comment.
Should we do the same on line 684?
I agree with the difficuly of testing this. It is unfortunate but the fix makes sense to me.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10970 +/- ##
=======================================
Coverage 96.18% 96.18%
=======================================
Files 178 178
Lines 19662 19662
=======================================
Hits 18911 18911
Misses 751 751
🚀 New features to boost your workflow:
|
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 6fed3d7 |
While at first it might make sense, we cannot guarantee that it is a directory. That is why I did not add it there. But, sitting here, thinking about it, we might want to. Perhaps it is worthy of a discussion on Discord?
Out of curiosity, a question. Where specifically would we put such a test? Would there be a file under tests/lint which already exists to which I would add such tests, would it be a new file there, or perhaps a new file under tests/functional/d? |
Why can we for the other case?
cinnion@6ad350f#diff-b6fcc07dde118fad26a433effbf0190d06193fbf57599f5e0619dd449cefdb9b has some examples for this I think :) It was one of the most recent changes to these lines, which also added tests. |
This fix is to
PyLinter._discover_files(). It will falsely ignore a with a name likeapplications_apiafter having previously walking across one namedapplications. The fix was to add the os directory separator to the end of what is a known directory (because of the fact that it has a file named__init__.pyin it).Type of Changes
| ✓ | 🐛 Bug fix |
Description
This fix is to
PyLinter._discover_files(). It will falsely ignore a with a name likeapplications_apiafter having previously walking across one namedapplications. The fix was to add the os directory separator to the end of what is a known directory (because of the fact that it has a file named__init__.pyin it).I have run the test suites, and while there are failing tests, I have verified that those were failing before my fix. I have also written a towncrier for this fix, and followed what seems to be a convention of removing the .rst extension from the file.
Lastly,
_discover_files()is currently not under test. Because of its nature of traversing a filesystem and the project's use of pytest, we would be doing some pretty serious setup and mocking to test this. Doable, yes... but involved. If such testing is a gating criteria for this PR, please contact me on the Discord server to discuss where to put them, etc.Closes #10969