-
Notifications
You must be signed in to change notification settings - Fork 3.1k
linting. #2048
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
linting. #2048
Conversation
@@ -827,30 +828,24 @@ def get_page(cls, link, req, skip_archives=True, session=None): | |||
) | |||
except requests.Timeout: | |||
cls._handle_fail(req, link, "timed out", url) | |||
except SSLError as exc: |
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.
This was a weird one. pylint pointed out that SSLError is a descendant class of ConnectionError, above, so this case can never be hit.
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.
so the SSLError could go before ConnectionError but I'm not sure it's worth it to have a special case for SSL
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.
If I'm preserving status-quo behavior, this is the right refactor.
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.
Could file a seperate bug if this erroneous behaviour.
1b664cc
to
e26337e
Compare
req, link, reason, url, | ||
level=2, | ||
meth=logger.info, | ||
) | ||
else: | ||
return inst | ||
|
||
@staticmethod | ||
def _handle_fail(req, link, reason, url, level=1, meth=None): |
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.
looks like url and level are useless here :o
78019a8
to
14d421d
Compare
This is good to go imo. I realize that the consensus seems to be against pylint, but this patch does improve the quality constraints provided by CI, and provides a path to incrementally improve things. |
So I started to poke at pylint, and it chokes on |
I've updated the branch to show how i'd make pip.baseparser pylint-clean. |
I'm going to close this for now. If at some point the distutils import error gets fixed we can revisit using pylint. |
pip.index is now pylint clean given a couple #FIXME:pylint:disable comments.
If i'm successful in my plans, my next few refactors should fix those.