Skip to content
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

Reduce pylint/flake8/Pyright/black/bandit errors #2120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gasull
Copy link

@gasull gasull commented Jan 1, 2021

Reduce pylint/flake8/Pyright/black/bandit errors to a minimum.

As so many errors are shown in IDEs, it becomes distracting and we risk ignoring serious bugs within the noise.

Among other things, this reorganizes imports, removing unused ones, breaks most long lines, and standardizes quotes as preferred by black.

Remove seed variable in run_offline_command because it's never accessed.

Add booleans is_storage_pw_req and is_wallet_pw_req for better readability.


This change is Reviewable

Reduce pylint/flake8/Pyright/black/bandit errors to a minimum.

As so many errors are shown in IDEs, it becomes distracting and we risk
ignoring serious bugs within the noise.

Among other things, this reorganizes imports, removing unused ones, breaks most long lines, and standardizes quotes as preferred by `black`.

Remove `seed` variable in `run_offline_command` because it's never accessed.

Add booleans `is_storage_pw_req` and `is_wallet_pw_req` for better
readability.
electron-cash Outdated Show resolved Hide resolved
electron-cash Show resolved Hide resolved
@cculianu
Copy link
Collaborator

cculianu commented Jan 1, 2021

I understand your motivation for submitting this PR -- and all of your help is very much appreciated. I do agree that minimal lint warnings in IDEs does make it easier to detect actual problems in the code. For new code we write I do recommend we stick to the latest linting craze that Python people have decided is what they prefer. No resistance there on that.

But -- this type of commit where we update an ancient file that nobody really changes much -- is not a great idea in my opinion. Commits such as these risk introducing new bugs and incompatibilities. Given that the rest of this app is by no means compliant with the latest pylint fashion anyway -- it's not like that much is gained by linting this file in this way. On top of that, just personal taste-wise -- I personally am not a huge fan of some aspects of the latest strict python linting fashion (and it really is a fashion). Things like insisting on " over ' to me is just Python programmers these days deciding what is fashionable. The language allows for both styles. Therefore it's up to the individual programmer and his mood on that particular line of code as to what he/she wants to use.

That being said --- this particular file -- electron-cash is not a very critical piece to the app. It's hardly updated -- we only add stuff to it when we need to but it's not a core piece of infra for the app.

I do agree its code was nasty in the past and is still nasty. I didn't write it -- it's not my code. It's hugely complex for what it is.

I would prefer we just not open up more cans of worms for ourselves. We risk introducing new bugs here.. and the practical benefit the way I see it is not very large, considering that this file is rarely updated anyway.

Lazily import `websockets` and add a comment explaining it's because
`SimpleWebSocketServer` isn't always available.
@gasull
Copy link
Author

gasull commented Jan 2, 2021

I personally am not a huge fan of some aspects of the latest strict python linting fashion (and it really is a fashion). Things like insisting on " over ' to me is just Python programmers these days deciding what is fashionable.

I personally don't love some choices black makes, but I do think it's good that it is standardized so most code looks similar and becomes easier to read and visually parse.

My main motivation is that clean code attracts new developers, in quantity and quality. And having more developers coming to BCH is a good thing.

That being said --- this particular file -- electron-cash is not a very critical piece to the app.

I didn't know where to start, and the executable file seemed like the obvious choice. I'm trying to do "Right Work" , as opposed to Good Work or even Great Work. Please see the linked post to understand what I mean.

I would prefer we just not open up more cans of worms for ourselves. We risk introducing new bugs here.. and the practical benefit the way I see it is not very large, considering that this file is rarely updated anyway.

I'm not planning more cleanup on this file. If you think it's too risky to merge this I'll understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants