-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Regression tests for 73 (distutil import error) #5620
Conversation
Pull Request Test Coverage Report for Build 1640977431
π - Coveralls |
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.
Was a bit quick there with the approval. Would it make sense to rename the test file? Most other regression tests start with regression_
I believe.
from distutils.util import strtobool | ||
|
||
print(distutils.version) | ||
print(strtobool) |
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.
DIdn't we also want to have a test to see that distutils
still emitted no-member
for incorrect imports? We might want to add one case here.
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.
""" | ||
Regression test to check that distutils can be imported |
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.
""" | |
Regression test to check that distutils can be imported | |
"""Regression test to check that distutils can be imported |
We need to find some pre-commit hook that starts to enforce this.
Perhaps we can look at something for Path
at the same time? π
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 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.
Better yet https://github.com/myint/docformatter
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.
For module docstrings I think both are fine. Those are tests after all.
Perhaps we can look at something for Path at the same time? π
I wouldn't recommend that. It's not really worth it and might limit us more than it actually would help.
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.
I wouldn't recommend that. It's not really worth it and might limit us more than it actually would help.
This was a little joke as @Pierre-Sassoulas keeps having to remind me to use it on the PRs I open π
Don't think we should enforce it all the time indeed.
Perhaps those packages allow us to exclude modules? Oh well, something to explore another time.
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.
Yes we definitely don't want to refactor all the old code that uses os.path.
I renamed :) Although I think having a nice explicit name is better than "regression_1234" so I kept the explicit part. |
@@ -57,7 +57,7 @@ class MessageLocationTuple(NamedTuple): | |||
|
|||
|
|||
class ManagedMessage(NamedTuple): | |||
"""Tuple with information ahout a managed message of the linter""" | |||
"""Tuple with information about a managed message of the linter""" |
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.
Perhaps clean up the commit history and don't squash?
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.
You're right I wanted to rebase originally but there are other commits that prevents that now.
Following change in the way distutil is stored in setuptools See pylint-dev/astroid#1321
22524d2
to
9a6afd3
Compare
Type of Changes
Description
Refer to pylint-dev/astroid#1321