Closed pdehaan closed 7 months ago
Also randomly tried bandit, which didn't give a lot of feedback:
bandit -r *.py emails phones privaterelay
[main] INFO profile include tests: None
[main] INFO profile exclude tests: None
[main] INFO cli include tests: None
[main] INFO cli exclude tests: None
[main] INFO running on Python 3.7.4
[node_visitor] INFO Unable to find qualified name for module: manage.py
Run started:2020-04-14 16:40:05.061205
Test results:
>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
Severity: Low Confidence: High
Location: emails/tests/model_tests.py:18
More Info: https://bandit.readthedocs.io/en/latest/plugins/b101_assert_used.html
17 relay_address = RelayAddress.make_relay_address(self.user)
18 assert relay_address.user == self.user
19
--------------------------------------------------
>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
Severity: Low Confidence: High
Location: emails/tests/model_tests.py:25
More Info: https://bandit.readthedocs.io/en/latest/plugins/b101_assert_used.html
24 relay_addresses.append(RelayAddress.make_relay_address(self.user))
25 assert len(relay_addresses) == len(set(relay_addresses))
26
--------------------------------------------------
>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
Severity: Low Confidence: High
Location: emails/tests/model_tests.py:32
More Info: https://bandit.readthedocs.io/en/latest/plugins/b101_assert_used.html
31 deleted_count = DeletedAddress.objects.filter(address_hash=address_hash).count()
32 assert deleted_count == 1
33
--------------------------------------------------
Code scanned:
Total lines of code: 1135
Total lines skipped (#nosec): 0
Run metrics:
Total issues (by severity):
Undefined: 0.0
Low: 3.0
Medium: 0.0
High: 0.0
Total issues (by confidence):
Undefined: 0.0
Low: 0.0
Medium: 0.0
High: 3.0
Files skipped (0):
@jrbenny35 offered the following advice in Slack:
black has become a standard but it's good to also have flake8
Flake8 output:
# Ignore "E501 line too long" errors.
flake8 *.py emails phones privaterelay --extend-ignore=E501
emails/apps.py:8:9: F401 'emails.signals' imported but unused
emails/views.py:9:1: F401 'decouple.config' imported but unused
emails/views.py:18:1: F401 'django.shortcuts.render' imported but unused
emails/views.py:18:1: F401 'django.shortcuts.get_object_or_404' imported but unused
emails/views.py:22:1: F401 '.models.DeletedAddress' imported but unused
emails/migrations/0004_auto_20190612_2047.py:2:1: F401 'uuid' imported but unused
emails/tests/model_tests.py:38:9: F841 local variable 'deleted_address' is assigned to but never used
phones/tests.py:1:1: F401 'django.test.TestCase' imported but unused
phones/views.py:11:1: F401 'django.shortcuts.render' imported but unused
phones/views.py:58:5: F841 local variable 'db_session' is assigned to but never used
phones/views.py:68:5: F841 local variable 'pretty_from' is assigned to but never used
phones/views.py:117:5: F841 local variable 'new_participant' is assigned to but never used
phones/views.py:125:5: F841 local variable 'message' is assigned to but never used
privaterelay/settings.py:150:69: E231 missing whitespace after ','
privaterelay/settings.py:188:78: E231 missing whitespace after ','
privaterelay/settings.py:189:79: E231 missing whitespace after ','
privaterelay/settings.py:190:80: E231 missing whitespace after ','
privaterelay/settings.py:255:70: E231 missing whitespace after ','
privaterelay/settings.py:256:60: E231 missing whitespace after ','
privaterelay/urls.py:16:1: F401 'decouple.config' imported but unused
privaterelay/views.py:38:5: F841 local variable 'c' is assigned to but never used
Sorry, I'm basically just using this issue as a notepad for my learnings at this point. Feel free to unsubscribe to my TED talk here...
@hackebrot also suggested the following:
I recommend using black for auto-formatting, mypy as a linter if you use type-annotations, flake8 for general linting, and pytest for automated tests.
The first tests are using pytest
already, and flake8
will be a good addition. We don't use type annotations, though. Let's start with that and maybe add black
if needed.
Also, digging in a bit more, I think https://github.com/pyupio/safety would be nice to add as well. It seems to be the Python verison of npm audit. Looks like we might need to update django.
https://github.com/vintasoftware/python-linters-and-code-analysis
I'm still playing w/ flake8-black, but not sure how valuable it is as a flake8 plugin vs standalone tool since flake8 only seems to give me vague output like:
./emails/models.py:15:16: BLK100 Black would make changes.
Here was my local .flake8 config file (but I honestly don't know enough about Python to know if thats the best approach, or if you want it some awkward tox file or add a precommit hook, etc):
[flake8]
exclude = */migrations/*,venv,.git
extend-ignore = E501
max-complexity = 10
I'm working on black + flake8 + mypy:
I think we can do this rapidly at the end of this sprint / beginning of next, so I've also moved MPP-79 to the current sprint.
1 of 5 linters integrated:
I think I'll also want a (optional) pre-commit hook, so devs can see linting exceptions before committing and submitting PRs.
The mypy
PR is a bit stuck, and others have not fully adopted black
, so I'm going to change tactics and introduce mypy
in person or over Zoom, so that the PR review is not the first encounter.
mypy
was merged at the end of June 2022. The next goals are to get developers to use it, and to increase strictness.
Many Python projects are switching to ruff
and enjoying the fast linting: https://beta.ruff.rs/docs/. This may be better than flake8
/ isort
.
Ref: #160
OK, I had a bit more time last night to randomly look at Python libraries, and maybe we want some combination of Black (#160) and Pylint (which I'm assuming is roughly similar to Prettier+ESLint respectively).
I managed to pip install pylint in a virtualenv and then run this command:
Where I had to do some fiddling with the .pylintrc config file to exclude a bunch of messages I wasn't ready to hear from yet (about 98% of that list was automatically generated by pylint, I only added a few gems at the top of the list):