-
Notifications
You must be signed in to change notification settings - Fork 79
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
Replace flake8
with ruff
#1101
Replace flake8
with ruff
#1101
Conversation
@@ -19,9 +19,9 @@ def _value(self): | |||
else: | |||
return self.data and self.data.strftime(self.format) or "" | |||
|
|||
def process_formdata(self, valuelist): |
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.
valuelist
is an un-pythonic name.
args: | ||
- --fix | ||
- --select=B,C,E,F,W | ||
- --ignore=C901,E203,E402,E501,W391,E261 |
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.
Ignored C901
: https://www.flake8rules.com/rules/C901.html
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 looks like a nice improvement. Thanks!
form_links = [link for link in form.data["links"] if link["url"]] | ||
for link in form_links: |
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 sure I would agree with the changes in this file, but also don't feel strongly about it. Was this something ruff suggested?
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.
The changes were made due to this rule. It called out 10+ other functions that had more than 10 if/else statements, and I changed the ones that could be done with the least amount of re-working.
If you'd like to see the other functions, you could remove C901
from the ignore list and rerun the linter.
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.
I think we could reimplement C901
in another PR, but it would require a lot of changes:
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1
OpenOversight/app/commands.py:183:5: C901 `update_officer_from_row` is too complex (14 > 10)
OpenOversight/app/commands.py:473:5: C901 `bulk_add_officers` is too complex (14 > 10)
OpenOversight/app/csv_imports.py:156:5: C901 `_handle_assignments_csv` is too complex (18 > 10)
OpenOversight/app/main/model_view.py:113:9: C901 `edit` is too complex (11 > 10)
OpenOversight/app/main/views.py:745:5: C901 `edit_department` is too complex (15 > 10)
OpenOversight/app/main/views.py:883:5: C901 `list_officer` is too complex (18 > 10)
OpenOversight/app/main/views.py:1374:5: C901 `label_data` is too complex (11 > 10)
OpenOversight/app/models/database_imports.py:102:5: C901 `update_officer_from_dict` is too complex (11 > 10)
OpenOversight/app/utils/forms.py:273:5: C901 `filter_by_form` is too complex (21 > 10)
Found 9 errors.
black....................................................................Passed
mypy.....................................................................Passed
args: | ||
- --ignore=E203,E402,E501,E800,W503,W391,E261,W504 | ||
- --select=B,C,E,F,W,T4,B9 | ||
- --exclude=./OpenOversight/app/db_repository/versions/ |
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 folder no longer exists, so I removed the exclude
.
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.
I haven't had time to review all the changes in this PR but I want to push back against large refactoring changes like this without good reason.
The previous djlint change introduced several minor bugs which weren't surfaced until the changes were live in production due to the scale of the refactoring. Comparing the build times, this ruff change built in 5m 26s while an earlier build took 4m 30s.
I mention this because our fork will need to pull in changes like this to keep in sync with the upstream. I definitely don't mean to block adding important guardrails but wanted to get a temp check since we expect ruff to have rough parity with our current setup and aren't seeing the performance enhancements.
@sea-kelp, I definitely get the hesitancy. I BELIEVE most every part of the code modified here is covered in test cases. Would you feel more comfortable with these changes if I were to verify that every line change was covered by tests? I also think there's a difference between changes in HTML versus Python syntax changes. The Python changes can be verified through a handful of mechanisms that are generally unavailable to HTML changes. Most of the changes here are also syntax changes that are more in line with Python best practices, which I think is of value as well (e.g., If there's a specific change you're curious or have questions about, I'd certainly be willing to go more into it. |
Thanks for taking a look, @sea-kelp! I'll let you know when the changes are made. |
Description of Changes
Replacing
flake8
withruff
, which has approximate parity and is more than 10x faster: https://docs.astral.sh/ruff/faq/#how-does-ruffs-linter-compare-to-flake8Tests and Linting
develop
branch.pytest
passes on my local development environment.pre-commit
passes on my local development environment.