kdmukai
commented
1 year ago Concept ACK but I'd still want some hands-on time to see it in action in my local dev.
I like that the warnings and edits here are strictly about code and not about formatting.
I've used Black for other projects for PEP-8 conformance but would prefer not to have a pre-commit enforce certain things like whitespace rules or line length reformatting. I have certain opinionated preferences (3 lines between classes or root-level defs, 2 lines between defs within a class, etc) that aren't strictly PEP-8, some of which I've been able to adjust in the past via linter config (with some struggle). And while I try to stick to a roughly 90-char line limit, some lines are just more readable if allowed to run long.
dbast
Description
pre-commit: The pre-commit framework became pretty widely use in recent years, especially in the Python world, where it originates from. There are many hooks for different routine checks like linting and code formatting, see also https://pre-commit.com/hooks.html. The hooks are configured in a file called
.pre-commit-config.yamland so don't pollute therequirements*.txtfiles. pre-commit locally installed via pip and can be added as git pre-commit hook via runningpre-commit installor just be executed on cli (or IDE on very file change) viapre-commit run --all. Added this as comments to the config file. With clever selections of hooks a runtime of <5 seconds can be achieved for a good developer experience, when running on every change. The config contains some standard hooks and ruff as Python code linter.ruff: https://github.com/astral-sh/ruff is proposed as Python linter as it is super fast (due to implemented in Rust) and implements lots of lints/tests from flake8/bandit/pyupgrade/pylint and more and more big projects are starting to adopt it.
This PR introduces pre-commit as concept and ruff with only the test category "F" enabled to keep the amount of fixes reviewable. An added GH workflow runs it on every PR to check that there no findings introduced.
The category "F" already finds lots of things, that are fixed by this PR, like duplicated imports, un-used return-values, un-reachable code, un-used variables etc, see bellow for a full list. One exception are the type annotations in the controller.py, which are non-standard: Decided to add
# noqaso that ruff ignores those and resolving those can be tried in a followup PR.This PR should contain no real functional changes, though it still requires very careful reviews.
Further PRs can add more hooks and/or enable more ruff test categories, if sensible.
Click me to see what is fixed
``` src/seedsigner/controller.py:55:13: F841 [*] Local variable `last` is assigned to but never used src/seedsigner/controller.py:100:16: F821 Undefined name `SeedStorage` src/seedsigner/controller.py:105:12: F821 Undefined name `embit` src/seedsigner/controller.py:106:17: F821 Undefined name `Seed` src/seedsigner/controller.py:107:19: F821 Undefined name `PSBTParser` src/seedsigner/controller.py:111:34: F821 Undefined name `embit` src/seedsigner/controller.py:132:19: F821 Undefined name `ScreensaverScreen` src/seedsigner/controller.py:133:33: F821 Undefined name `BaseToastOverlayManagerThread` src/seedsigner/controller.py:208:43: F821 Undefined name `Seed` src/seedsigner/controller.py:397:53: F821 Undefined name `BaseToastOverlayManagerThread` src/seedsigner/gui/__init__.py:1:23: F401 [*] `.renderer.Renderer` imported but unused src/seedsigner/gui/components.py:342:70: F541 [*] f-string without any placeholders src/seedsigner/gui/components.py:1349:5: F841 [*] Local variable `start` is assigned to but never used src/seedsigner/gui/keyboard.py:2:35: F401 [*] `PIL.ImageFont` imported but unused src/seedsigner/gui/keyboard.py:321:9: F841 [*] Local variable `key` is assigned to but never used src/seedsigner/gui/keyboard.py:555:13: F841 [*] Local variable `cursor_block_height` is assigned to but never used src/seedsigner/gui/renderer.py:4:39: F401 [*] `seedsigner.gui.components.Fonts` imported but unused src/seedsigner/gui/renderer.py:4:46: F401 [*] `seedsigner.gui.components.GUIConstants` imported but unused src/seedsigner/gui/screens/__init__.py:1:1: F403 `from .screen import *` used; unable to detect undefined names src/seedsigner/gui/screens/psbt_screens.py:9:39: F401 [*] `.screen.WarningScreen` imported but unused src/seedsigner/gui/screens/psbt_screens.py:10:38: F401 [*] `..components.Button` imported but unused src/seedsigner/gui/screens/psbt_screens.py:139:43: F541 [*] f-string without any placeholders src/seedsigner/gui/screens/psbt_screens.py:140:43: F541 [*] f-string without any placeholders src/seedsigner/gui/screens/psbt_screens.py:143:39: F541 [*] f-string without any placeholders src/seedsigner/gui/screens/psbt_screens.py:554:23: F541 [*] f-string without any placeholders src/seedsigner/gui/screens/scan_screens.py:12:33: F401 [*] `.screen.ButtonListScreen` imported but unused src/seedsigner/gui/screens/scan_screens.py:13:47: F401 [*] `..components.TextArea` imported but unused src/seedsigner/gui/screens/scan_screens.py:88:49: F401 [*] `timeit.default_timer` imported but unused src/seedsigner/gui/screens/screen.py:346:13: F841 [*] Local variable `center_x` is assigned to but never used src/seedsigner/gui/screens/screen.py:660:18: F821 Undefined name `EncodeQR` src/seedsigner/gui/screens/screen.py:663:41: F821 Undefined name `EncodeQR` src/seedsigner/gui/screens/seed_screens.py:178:9: F841 [*] Local variable `y` is assigned to but never used src/seedsigner/gui/screens/seed_screens.py:1188:9: F841 [*] Local variable `msg_height` is assigned to but never used src/seedsigner/gui/screens/seed_screens.py:1189:9: F841 [*] Local variable `msg_width` is assigned to but never used src/seedsigner/gui/screens/settings_screens.py:6:152: F401 [*] `seedsigner.gui.components.IconTextLine` imported but unused src/seedsigner/gui/screens/settings_screens.py:7:49: F401 [*] `seedsigner.gui.screens.scan_screens.ScanScreen` imported but unused src/seedsigner/gui/screens/settings_screens.py:9:43: F401 [*] `seedsigner.gui.screens.screen.BaseScreen` imported but unused src/seedsigner/hardware/pivideostream.py:5:8: F401 [*] `time` imported but unused src/seedsigner/helpers/mnemonic_generation.py:5:25: F401 [*] `embit.bip39.mnemonic_to_bytes` imported but unused src/seedsigner/helpers/mnemonic_generation.py:5:44: F401 [*] `embit.bip39.mnemonic_from_bytes` imported but unused src/seedsigner/helpers/ur2/bytewords.py:108:5: F841 [*] Local variable `body_checksum` is assigned to but never used src/seedsigner/helpers/ur2/bytewords.py:109:5: F841 [*] Local variable `checksum` is assigned to but never used src/seedsigner/helpers/ur2/fountain_decoder.py:9:20: F401 [*] `.utils.join_lists` imported but unused src/seedsigner/helpers/ur2/fountain_encoder.py:53:29: F841 [*] Local variable `err` is assigned to but never used src/seedsigner/helpers/ur2/ur_decoder.py:9:31: F401 [*] `.fountain_encoder.FountainEncoder` imported but unused src/seedsigner/helpers/ur2/ur_decoder.py:11:1: F403 `from .bytewords import *` used; unable to detect undefined names src/seedsigner/helpers/ur2/ur_decoder.py:46:16: F405 `Bytewords` may be undefined, or defined from star imports src/seedsigner/helpers/ur2/ur_decoder.py:46:33: F405 `Bytewords_Style_minimal` may be undefined, or defined from star imports src/seedsigner/helpers/ur2/ur_decoder.py:123:20: F405 `Bytewords` may be undefined, or defined from star imports src/seedsigner/helpers/ur2/ur_decoder.py:123:37: F405 `Bytewords_Style_minimal` may be undefined, or defined from star imports src/seedsigner/helpers/ur2/ur_decoder.py:138:29: F841 [*] Local variable `err` is assigned to but never used src/seedsigner/helpers/ur2/ur_encoder.py:9:1: F403 `from .bytewords import *` used; unable to detect undefined names src/seedsigner/helpers/ur2/ur_encoder.py:20:16: F405 `Bytewords` may be undefined, or defined from star imports src/seedsigner/helpers/ur2/ur_encoder.py:20:33: F405 `Bytewords_Style_minimal` may be undefined, or defined from star imports src/seedsigner/helpers/ur2/ur_encoder.py:47:16: F405 `Bytewords` may be undefined, or defined from star imports src/seedsigner/helpers/ur2/ur_encoder.py:47:33: F405 `Bytewords_Style_minimal` may be undefined, or defined from star imports src/seedsigner/models/decode_qr.py:422:33: F841 [*] Local variable `e` is assigned to but never used src/seedsigner/models/decode_qr.py:506:9: F811 Redefinition of unused `is_sign_message` from line 286 src/seedsigner/models/decode_qr.py:559:37: F541 [*] f-string without any placeholders src/seedsigner/models/decode_qr.py:573:29: F541 [*] f-string without any placeholders src/seedsigner/models/decode_qr.py:576:29: F541 [*] f-string without any placeholders src/seedsigner/models/decode_qr.py:579:29: F541 [*] f-string without any placeholders src/seedsigner/models/decode_qr.py:582:29: F541 [*] f-string without any placeholders src/seedsigner/models/decode_qr.py:790:33: F841 [*] Local variable `e` is assigned to but never used src/seedsigner/models/decode_qr.py:818:33: F841 [*] Local variable `e` is assigned to but never used src/seedsigner/models/decode_qr.py:841:33: F841 [*] Local variable `e` is assigned to but never used src/seedsigner/models/encode_qr.py:8:19: F811 Redefinition of unused `bip32` from line 3 src/seedsigner/models/encode_qr.py:9:28: F811 Redefinition of unused `NETWORKS` from line 4 src/seedsigner/models/psbt_parser.py:65:19: F541 [*] f-string without any placeholders src/seedsigner/models/seed_storage.py:40:40: F841 [*] Local variable `e` is assigned to but never used src/seedsigner/views/__init__.py:1:1: F403 `from .view import *` used; unable to detect undefined names src/seedsigner/views/scan_views.py:5:43: F401 [*] `seedsigner.gui.screens.screen.RET_CODE__BACK_BUTTON` imported but unused src/seedsigner/views/scan_views.py:10:98: F401 [*] `seedsigner.views.view.OptionDisabledView` imported but unused src/seedsigner/views/scan_views.py:181:31: F541 [*] f-string without any placeholders src/seedsigner/views/screensaver.py:137:9: F841 [*] Local variable `screensaver_start` is assigned to but never used src/seedsigner/views/seed_views.py:17:32: F811 Redefinition of unused `embit_utils` from line 13 src/seedsigner/views/seed_views.py:275:18: F541 [*] f-string without any placeholders src/seedsigner/views/seed_views.py:1095:29: F541 [*] f-string without any placeholders src/seedsigner/views/seed_views.py:1235:29: F541 [*] f-string without any placeholders src/seedsigner/views/tools_views.py:1:25: F401 [*] `dataclasses.dataclass` imported but unused src/seedsigner/views/tools_views.py:283:23: F541 [*] f-string without any placeholders tests/screenshot_generator/generator.py:5:24: F401 [*] `mock.patch` imported but unused tests/screenshot_generator/generator.py:8:40: F401 [*] `seedsigner.models.settings.Settings` imported but unused tests/screenshot_generator/generator.py:25:41: F401 [*] `seedsigner.hardware.buttons.HardwareButtons` imported but unused tests/screenshot_generator/generator.py:26:40: F401 [*] `seedsigner.hardware.camera.Camera` imported but unused tests/screenshot_generator/generator.py:238:14: F541 [*] f-string without any placeholders tests/screenshot_generator/generator.py:272:19: F541 [*] f-string without any placeholders tests/test_bip85.py:1:8: F401 [*] `pytest` imported but unused tests/test_bip85.py:2:18: F401 [*] `mock.MagicMock` imported but unused tests/test_bip85.py:4:19: F401 [*] `embit.bip39` imported but unused tests/test_bip85.py:6:40: F401 [*] `seedsigner.models.settings.SettingsConstants` imported but unused tests/test_controller.py:25:13: F841 [*] Local variable `c` is assigned to but never used tests/test_decodepsbtqr.py:286:5: F841 [*] Local variable `bad2` is assigned to but never used tests/test_decodepsbtqr.py:287:5: F841 [*] Local variable `bad3` is assigned to but never used tests/test_decodepsbtqr.py:299:5: F841 [*] Local variable `main_bech32_address3` is assigned to but never used tests/test_embit_utils.py:377:9: F601 Dictionary key literal `(None, SC.CUSTOM_DERIVATION, None, 5)` repeated tests/test_encodepsbtqr.py:20:9: F841 [*] Local variable `img` is assigned to but never used tests/test_encodepsbtqr.py:58:9: F841 [*] Local variable `img` is assigned to but never used tests/test_flows_psbt.py:3:35: F401 [*] `seedsigner.controller.Controller` imported but unused tests/test_flows_seed.py:11:80: F401 [*] `seedsigner.views.view.RemoveMicroSDWarningView` imported but unused tests/test_flows_seed.py:12:70: F401 [*] `seedsigner.views.tools_views` imported but unused tests/test_flows_seed.py:196:76: F841 [*] Local variable `e` is assigned to but never used tests/test_seed.py:3:40: F401 [*] `seedsigner.models.settings.SettingsConstants` imported but unused tests/test_seedqr.py:2:8: F401 [*] `pyzbar` imported but unused tests/test_seedqr.py:5:46: F401 [*] `seedsigner.helpers.ur2.bytewords.decode` imported but unused tests/test_seedqr.py:9:40: F401 [*] `seedsigner.models.settings.SettingsConstants` imported but unused ```This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os:
Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.