Fix flake8 errors #610
Fix flake8 errors #610
Conversation
|
Note: Most of the fixes (especially fixes about blank line and white space errors) are generated by autopep8 |
CendioOssman
left a comment
CendioOssman
left a comment
There was a problem hiding this comment.
Nice cleanups!
Have you explored getting this in to GitHub actions as well? So we don't regress.
kajinamit
force-pushed
the
flake8
branch
2 times, most recently
from
92796b6 to
0bd84b8
Compare
I've added the workflow to run pep8 check. There are still number of checks disabled but at least we can detect any new errors. Does it make sense to you ? We can probably use tox for the existing unit tests but I'll leave it for now. |
websockify/token_plugins.py
Outdated
| index = 1 | ||
| for f in cfg_files: | ||
| for line in [l.strip() for l in open(f).readlines()]: | ||
| for line in [raw_line.strip() for raw_line in open(f).readlines()]: |
There was a problem hiding this comment.
Is this a flake8 rule? I'd say it's common (desired?) to use a short name when doing list comprehension.
There was a problem hiding this comment.
flake8 does not allow ambiguous variable name and detects the following error.
./websockify/token_plugins.py:55:40: E741 ambiguous variable name 'l'
| from websockify.websocket import * # noqa: F401,F403 | ||
| from websockify.websocketproxy import * # noqa: F401,F403 |
There was a problem hiding this comment.
Can't these be easily fixed instead?
There was a problem hiding this comment.
IIUC the intention of this is to allow importing all objects from these two submodules. We can use explicit import and probably __all__ to avoid errors but I think that's overcomplicated.
websockify/websocketproxy.py
Outdated
| # Save off proxy specific options | ||
| self.target_host = kwargs.pop('target_host', None) | ||
| self.target_port = kwargs.pop('target_port', None) | ||
| self.wrap_cmd = kwargs.pop('wrap_cmd', None) | ||
| self.wrap_mode = kwargs.pop('wrap_mode', None) | ||
| self.wrap_cmd = kwargs.pop('wrap_cmd', None) # noqa: E221 | ||
| self.wrap_mode = kwargs.pop('wrap_mode', None) # noqa: E221 | ||
| self.unix_target = kwargs.pop('unix_target', None) | ||
| self.ssl_target = kwargs.pop('ssl_target', None) | ||
| self.heartbeat = kwargs.pop('heartbeat', None) | ||
| self.ssl_target = kwargs.pop('ssl_target', None) # noqa: E221 | ||
| self.heartbeat = kwargs.pop('heartbeat', None) # noqa: E221 | ||
|
|
||
| self.token_plugin = kwargs.pop('token_plugin', None) | ||
| self.host_token = kwargs.pop('host_token', None) | ||
| self.auth_plugin = kwargs.pop('auth_plugin', None) | ||
| self.host_token = kwargs.pop('host_token', None) # noqa: E221 | ||
| self.auth_plugin = kwargs.pop('auth_plugin', None) # noqa: E221 |
There was a problem hiding this comment.
There was a problem hiding this comment.
It has been a while since we discussed this. I wonder if we can agree with following the pep8 guideline now (I mean, remove the adjustment commit for now) and "fix" these later if we want.
I know this is not urgent but I hope we can merge this soon, because this likely conflict with any new change, due to multiple lines being updated.
Sounds great! It allows us to lock down what we already agree on, and explore other things as we want/need. |
kajinamit
force-pushed
the
flake8
branch
2 times, most recently
from
ac750a7 to
1c0d086
Compare
|
So I think I addressed all of the points (except for ones which I believe we can't/shouldn't). |
... so that we can run flake8 check more easily. Note that the check is not yet enforced in CI.
print statement is no longer available in Python 3.
... and fix the wrong format of Sec-WebSocket-Protocol header in test case.
... instead of == or != .
... to fix "E265 block comment should start with '# '".
CendioOssman
left a comment
CendioOssman
left a comment
There was a problem hiding this comment.
Thanks for all the adjustments! I think this is pretty much ready to go.
There was one indentation change that looked strange, though. Could you have a look at that, after which we can merge this.
kajinamit
force-pushed
the
flake8
branch
2 times, most recently
from
03cac53 to
2d39629
Compare
|
Thanks for the additional feedback ! |
|
Ignore this. I noticed I didn't commit the additional change in tox.ini properly. |
... which are now complained by flake8.
... to ensure the logic works only valid exceptions.
... to avoid pep8 errors caused by implicit import of a few modules.
... to get rid of F401 error by flake8.
The existing code does not expect 79 characters limit. Extend the limit to avoid mass update.
CendioOssman
left a comment
CendioOssman
left a comment
There was a problem hiding this comment.
Looks great!
Thanks for doing all the hard work here!
2 participants
Fix flake8 errors to improve readability of the code and also avoid potential bugs.