Configure Flake8 and resolve errors #347
Conversation
Signed-off-by: Dan Callahan <danc@element.io>
Signed-off-by: Dan Callahan <danc@element.io>
Signed-off-by: Dan Callahan <danc@element.io>
|
This supersedes #345 |
|
|
||
| [flake8] | ||
| max-line-length = 88 | ||
| extend-ignore = E203,E501,F821 # TODO: Fix E501 and F821 |
There was a problem hiding this comment.
E501 is line length; we can deal with that separately.
F821 is undefined name... this occurs on two lines in sydent/http/federation_tls_options.py, and frankly I just need more time to understand context here, since that code cannot work as it is, but has been in place for a few years.
There was a problem hiding this comment.
Re: F821- so if a piece of code is broken but it's been there awhile the prudent thing to do is investigate it thoroughly rather than just fix/remove?
There was a problem hiding this comment.
Yep! See Chesterton's fence -- I don't know why that code is like that, and until I do, it's hard to know how it should be fixed. What I do know is that Sydent has been running with that code present for literally two years, so it can't be too problematic. So why?
Maybe its enclosing function is dead code? (Nope; I just checked and several paths eventually lead to it).
It's in an exception handler, so maybe the code it's wrapping never actually raises an exception, so control never passes into the except block? (Unlikely...)
Or maybe when it execution does flow into the except block it just... blows up with a NameError instead of properly handling things. (Likely!) Looking at the upstream Twisted code that this borrows from, it looks like its purpose is to just do a better job of logging an error, and then drop a connection... so quite possibly benign if it fails.
|
|
||
| [flake8] | ||
| max-line-length = 88 | ||
| extend-ignore = E203,E501,F821 # TODO: Fix E501 and F821 |
There was a problem hiding this comment.
Re: F821- so if a piece of code is broken but it's been there awhile the prudent thing to do is investigate it thoroughly rather than just fix/remove?
| @@ -54,7 +54,7 @@ def _createSchema(self): | |||
| try: | |||
| logger.info("Importing %s", scriptPath) | |||
| c.executescript(fp.read()) | |||
| except: | |||
| except Exception: | |||
There was a problem hiding this comment.
Exception here is the base class of exceptions and so should just function to catch any-typed exceptions?
There was a problem hiding this comment.
Yep! See Flake8 Rules for a really good explanation, and the Python 3 Exception Hierarchy
| @@ -60,7 +60,7 @@ def get_json(self, uri, max_size=None): | |||
| try: | |||
| # json.loads doesn't allow bytes in Python 3.5 | |||
| json_body = json_decoder.decode(body.decode("UTF-8")) | |||
| except Exception as e: | |||
There was a problem hiding this comment.
I left this-I wasn't quite sure how the e was functioning (other than storing an exception if thrown?)
There was a problem hiding this comment.
The e is just there to create a variable which points to the actual instance of the Exception you caught. Useful if you want to actually do something with the specific exception (programmatically inspecting it, etc.)
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.