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

Flake8 all the things #974

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dorianpula
Contributor

dorianpula commented May 30, 2017

Introduce and make sure the Paramiko code passes Flake 8 check.
The setup is similar to what Invoke and Fabric use.

Notes:

  • Tests and demo code is not included.
  • Tried to avoid refactoring code unless it was absolutely necessary.
@bitprophet

This comment has been minimized.

Member

bitprophet commented May 31, 2017

Thanks again for tackling this! So excited.

Wishing in retrospect I'd had you start on the 1.17 or 1.18 branch, but nbd - this is the lion's share of the work - I can take care of trying to backport (or just...not...rolling forwards is a thing.)

I'll muck around with this momentarily, add to Travis, etc.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

Rebasing to 2.0 locally, going beyond that seems a fool's errand. Then layering more modern/house style on top, mostly removing backslash line continuations in favor of parentheticals.

@bitprophet bitprophet closed this in a46aaca Jun 1, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

Also had to tweak flake8 version to work on 2.6...still waiting to see if that is enough to please the Travis gods.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

Green yey

@@ -297,7 +300,6 @@ def _parse_service_accept(self, m):
maj_status = m.get_int()
min_status = m.get_int()
err_msg = m.get_string()
lang_tag = m.get_string() # we don't care!

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

I guess this can be dropped because an exception is immediately raised?

(It's worth pointing out the non-whitespace changes for a more careful review.)

self.transport._log(
WARNING,
'Auth rejected because the client attempted to change '
'username in mid-flight')

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

In some projects it's allowed to exceed the line-length for log messages and the like, so that if you e.g. grep for something like "change username in mid-flight" after seeing it in a log line, you find it.

@@ -566,7 +604,6 @@ def _parse_userauth_failure(self, m):
def _parse_userauth_banner(self, m):
banner = m.get_string()
self.banner = banner
lang = m.get_string()

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

just pointing out another not-just-whitespace change (which is probably OK)
(but you can't always drop m.get_string() when the result is unused, because it mutates m)

This comment has been minimized.

@bitprophet

bitprophet Jun 1, 2017

Member

Yea this feels like an oops to me, I think probably best to be consistent with the change in kex_gss.py, where we just do a discarded m.get_string().

def __getitem__(self, key):
ret = self.lookup(key)
if ret is None:
raise KeyError(key)
return ret
def __delitem__(self, key):
pass # Needed for instantiating HostKeys.

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

a minor functional change here - previously an exception would be thrown if the key was not found, now it won't

This comment has been minimized.

@bitprophet

bitprophet Jun 1, 2017

Member

Yea we couldn't figure out what the heck was going on with the original change (happened in the Python 3 shakeup). Originally we just nuked it but I guess Dorian found it was added on purpose. I don't really see why we couldn't make it actually do del self[key] tho...

@@ -271,7 +281,7 @@ def _parse_kexgss_error(self, m):
maj_status = m.get_int()
min_status = m.get_int()
err_msg = m.get_string()
lang_tag = m.get_string() # we don't care about the language!
m.get_string() # we don't care about the language!

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

interesting that unused m string was handled differently here (even though it also immediately raises an exception)

@@ -301,9 +304,8 @@ def enable_auth_gssapi(self):
:see: : `.ssh_gss`
"""
UseGSSAPI = False
GSSAPICleanupCredentials = False

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

this is an odd one - also set in a test and an example, but never used anywhere (removal looks correct)

This comment has been minimized.

@bitprophet

bitprophet Jun 1, 2017

Member

If it's set in test and example but not a functional API member, should probably nuke from those latter as well.

self.__transport._log(
ERROR,
'Exception in subsystem handler for "{}": {}'.format(
self.__name, e))

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

a format style change, uncommon in this PR

This comment has been minimized.

@bitprophet

bitprophet Jun 1, 2017

Member

But not a bad change IMHO, {} > %s.

@@ -699,7 +715,7 @@ def getfo(self, remotepath, fl, callback=None):
reader=fr, writer=fl, file_size=file_size, callback=callback
)
return size
return file_size

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

looks like a bug fix

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

... in fact it's just extraneous, the real return is above in the with: context. if that fails, an exception will be thrown. this return will never be hit

(but it's fine to just appease flake8 for now)

This comment has been minimized.

@bitprophet

bitprophet Jun 1, 2017

Member

I'd rather just nuke it now that we've figured that out, tbh :D

# can not rewrite this to deal with E721, either as a None check
# nor as not an instance of None or NoneType
if fileobj is not type(None): # noqa

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

good catch here

raise Exception('unknown type for ' + repr(item) + ' type ' + repr(type(item)))
raise Exception(
'unknown type for {!r} type {!r}'.format(
item, type(item)))

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

interesting format style change (looks fine)

This comment has been minimized.

@bitprophet

bitprophet Jun 1, 2017

Member

👍 here

@@ -17,6 +17,7 @@
# along with Paramiko; if not, write to the Free Software Foundation, Inc.,
# 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
# flake8: noqa

This comment has been minimized.

@ploxiln

ploxiln Jun 1, 2017

Contributor

a bunch of changes were made below to appease flake8, is this a leftover from partway through the process?

This comment has been minimized.

@bitprophet

bitprophet Jun 1, 2017

Member

Seems likely, let's nuke it.

This comment has been minimized.

@dorianpula

dorianpula Jun 1, 2017

Contributor

Yeah, I tried doing Flake8 for this module, and it was just overwhelming. Hence the cop-out and I skipped checking the entire module. A better approach would be to break out some of the deeply nested code into separate functions, but I wanted to concentrate on doing this in a separate PR.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

Thanks for the post-review, @ploxiln - I admit I took most of it on faith due to size & having been physically present during much of the work 😁

I've replied to most of the inline notes & I'll go through and tidy them all up soon unless somebody else beats me to it. (I supplied a lot of similar post-merge changes yesterday, as well, with lots of style tweaks like removal of most line continuations.)

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jun 1, 2017

I did notice your follow-up style changes - they did improve the line-wrapping considerably :)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

I was unpleasantly surprised at how many I had to leave in, though, because I honestly couldn't say the alternatives were actually better. Usually indicative of deeper code smells that required more time to truly fix!

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

Working on those updates now.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

Ah, the flake8: noqa in transport.py was because there's still about 89 violations in the file (mostly line length). Will see if it's something I can tackle or if it's worth leaving it as-is.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

OK that's all done now?! Waiting on Travis to agree.

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jun 1, 2017

👍

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Jun 1, 2017

found bug in b99b234 - should not all be {0}

@@ -108,7 +109,7 @@ class BadHostKeyException (SSHException):
     .. versionadded:: 1.6
     """
     def __init__(self, hostname, got_key, expected_key):
-        message = 'Host key for server {} does not match: got {} expected {}'
+        message = 'Host key for server {0} does not match: got {0}, expected {0}' # noqa
         message = message.format(
             hostname, got_key.get_base64(),
             expected_key.get_base64())
@dorianpula

This comment has been minimized.

Contributor

dorianpula commented Jun 1, 2017

@bitprophet It looks like master will get green once the transport.py is fixed up. 😞

Is there anything else I can help with flake8? Or I can jump on something else...

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

@ploxiln thought I fixed all of those, will double check...EDIT: yup I did miss one, pushed fix, thx!

@dorianpula Don't think so, I think this is pretty much licked now!

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

Looks green

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment