-
Notifications
You must be signed in to change notification settings - Fork 377
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
Fixed all flake8 errors and added flake8 in the build script #169
Conversation
I did not choose the My system was using 160, and I known some prefer 79/80. I would ask to merge this as is, this way we can get the linter running, because there are 700 violations of the max-line-length as 79 and 645 as 80. |
Max line length is setup here. I would like us to try and stick to a soft-limit of 80 lines and a hard limit of 99 or 100. |
@hackaugusto you are riddling the codebase with a lot of Do more people use it? |
@@ -117,7 +117,11 @@ def __setattr__(self, name, value): | |||
|
|||
length = len(value) | |||
if length > field.size_bytes: | |||
raise ValueError('value with length {length} for {attr} is to big'.format(length=length, attr=name)) | |||
msg = 'value with length {length} for {attr} is to big'.format( |
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.
Since we are fixing style here, let's also fix this typo.
is to big
-> is too big
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.
Yep, nice catch :)
@@ -45,7 +45,8 @@ class ContractDiscovery(Discovery): | |||
"""On chain smart contract raiden node discovery: allows to register endpoints (host, port) for | |||
your ethereum-/raiden-address and looking up endpoints for other ethereum-/raiden-addressess. | |||
""" | |||
def __init__(self, blockchainservice, discovery_contract_address, poll_timeout=DEFAULT_POLL_TIMEOUT): | |||
def __init__(self, blockchainservice, discovery_contract_address, |
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 is a matter of style that we have discussed before. If the line breaks on a function's arguments then please format it like:
def __init__(
self,
blockchainservice,
discovery_contract_address,
poll_timeout=DEFAULT_POLL_TIMEOUT
):
Why? It's actually quite simple. First it looks symmetric, simple and organized. But for a more practical reason, when refactoring the code later and adding/removing arguments you just need to change one entry and don't have to mess with rearranging the entire arguments line.
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.
Humm, the suggestion doesnt fully comply with the Python's coding standard PEP8:
When using a hanging indent the following should be considered; there should be no arguments on the first line and further indentation should be used to clearly distinguish itself as a continuation line.
# More indentation included to distinguish this from the rest.
def long_function_name(
var_one,
var_two,
var_three,
var_four):
print(var_one)
Pylint is the tool erroring in this (the styling tools are not), so I will have a look on how to configure this (I have no preference among the hanging indent options).
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.
Oh I am fine with placing the closing parentheses after the last argument. What I really want to emphasize is each argument on its own line if you are to break any arguments.
But note that flake8 accepts both closing parentheses styles. I don't have any strong opinion on that. Whatever you prefer.
@@ -99,7 +105,8 @@ class BlockChainService(object): | |||
""" Exposes the blockchain's state through JSON-RPC. """ | |||
# pylint: disable=too-many-instance-attributes,unused-argument | |||
|
|||
def __init__(self, privatekey_bin, registry_address, host, port, poll_timeout=DEFAULT_POLL_TIMEOUT, **kwargs): | |||
def __init__(self, privatekey_bin, registry_address, host, port, |
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.
Same comment as above. Break all arguments each in its own line.
@@ -277,8 +284,10 @@ def uninstall(self): | |||
|
|||
|
|||
class Asset(object): | |||
def __init__(self, jsonrpc_client, asset_address, startgas=GAS_LIMIT, # pylint: disable=too-many-arguments | |||
def __init__(self, jsonrpc_client, asset_address, startgas=GAS_LIMIT, |
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.
Same comment as above. Break all arguments each in its own line.
@@ -410,8 +421,10 @@ def assetadded_filter(self): | |||
|
|||
|
|||
class ChannelManager(object): | |||
def __init__(self, jsonrpc_client, manager_address, startgas=GAS_LIMIT, # pylint: disable=too-many-arguments | |||
def __init__(self, jsonrpc_client, manager_address, startgas=GAS_LIMIT, |
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.
Same comment as above. Break all arguments each in its own line.
@@ -9,7 +9,9 @@ changedir = {toxinidir}/raiden | |||
deps = | |||
-rrequirements.txt | |||
pdbpp | |||
pep8 |
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.
Why pep8? I thought we had agreed on using flake8
for this ...
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.
flake8 is a wrapper around pep8/pyflakes/mccabe, I decided to start with the simpler option, in other words, these changes are required but not sufficient to use flake8 and I did not look at flake8 at this moment.
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.
Go with flake8 then, it's quite a complete tool and will help us a lot in keeping our styling checked. Moreover we need consistency, can't have each dev trying out a different tool.
commands = | ||
pep8 --exclude .eggs,.tox,devenv,tools/ansible . |
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.
Please use flake8
@@ -18,7 +20,9 @@ changedir = {toxinidir}/raiden | |||
deps = | |||
-rrequirements.txt | |||
coverage==4.0 | |||
pep8 |
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.
Please use flake8
commands = | ||
pep8 --exclude .eggs,.tox,devenv,tools/ansible . |
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.
Please use flake8
@@ -38,3 +42,6 @@ commands = flake8 raiden | |||
[flake8] | |||
max-line-length = 99 | |||
max-complexity = 10 | |||
|
|||
[pep8] | |||
max-line-length = 99 |
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.
Not needed, the flake8
configuration is just 3 lines above this.
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.
Hummm, if you don't mind these 3 extra lines I'm going to leave them there, pep8 is a requirement for the flake8 tool, that means both binaries are available if flake8 is installed and tools like syntastic will run both automatically, this makes life easier by having both tools consistent. Note: this happen even if pep8 is not installed directly and we are using just the pycodestyle package.
I wouldn't call it riddling.
It's a nice tool, I recommend you to give it a try. It does more checks than pep8/pyflakes/mccabe, it actually points out bad practices, but saddly has more false errors. |
Interesting, I didn't realize we had a second config file, I'm going to remove the configuration section from the |
@hackaugusto There are some minor conflicts. Can you address and rebase? |
The build will fail until PR 502 is merged. |
@@ -19,13 +19,14 @@ before_install: | |||
- export PATH=$PATH:~/.bin | |||
install: | |||
- pip install -r requirements.txt | |||
- pip install coveralls | |||
- pip install coveralls pep8 |
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.
@hackaugusto Then maybe remove the pep8 tool invocation from here until PyCQA/pycodestyle#502 is merged?
No reason to leave this PR hanging on our side. It does fix a lot of style issues.
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.
removed from travis.yml
, kept in tox.ini
.
@hackaugusto Looks good to me. Needs a rebase though. We can merge afterwards. |
- removed pep8 from the travis.yml until PyCQA/pycodestyle#502 is merged. - fixed one additional style issue
I've forced a rebuild on travis. The build is fine now. This can be rebased on master without conflicts. |
Note that I did not try to fix any flake8 erros, I assumed that the code is correct and only silenced it