Switch to ruff / Add pre-commit hook #4428

spreeni · 2024-06-09T23:26:19Z

I switched from black/isort/autoflake to ruff, which is much faster, combines these tools and also is a strong linter. Additionally, I set up a pre-commit hook that can optionally be used to enforce QA-checks before each commit. I personally find it very satisfying to work with this, as it often corrects/spots smaller errors.

I exclude stricter linting rules that might limit our development style, but applied multiple other linting rules which I think are sensible.

ruff linting modules that I normally use but omitted for now are pydocstyle ("D") and mccabe complexity ("C90"), as some functionality would need to be refactored to reduce complexity in our case and enforcing doc strings everywhere might also be quite restrictive.

Backend checklist

Formatted my code by running ruff check --select I --fix . && ruff check . && ruff format . in app/backend
Added tests for any new code or added a regression test if fixing a bug
All tests pass
Run the backend locally and it works
Added migrations if there are any database changes, rebased onto develop if necessary for linear migration history

… to media and client

vercel · 2024-06-09T23:26:23Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
couchers	✅ Ready (Inspect)	Visit Preview	Jun 11, 2024 7:41am

aapeliv

Neat! Definitely caught a few subtle bugs/issues here, thanks for introducing the linter! I even learned some more python syntax I wasn't even aware of!

Personally I don't mind the empty f-strings, and I doubt they really affect performance much. But I guess good to get rid of either way.

app/.gitlab-ci.yml

app/backend/pyproject.toml

app/backend/.pre-commit-config.yaml

app/backend/src/couchers/migrations/versions/bf12729fa8eb_sms_verification_constraints.py

app/backend/src/couchers/servicers/account.py

app/backend/src/couchers/servicers/search.py

app/backend/src/tests/test_fixtures.py

app/backend/src/tests/test_interceptors.py

aapeliv · 2024-06-10T01:24:26Z

app/backend/pyproject.toml

+ignore = [
+    "B007",   # Loop control variable not used within loop body
+    "E501",   # Line too long
+    "E711",   # Comparison to `None` should be `cond is None`
+    "E712",   # Avoid equality comparisons to `True`; use `if approved:` for truth checks
+    "F811",   # Redefinition of unused variable
+    "F841",   # Local variable is assigned to but never used
+    "UP015",  # Unnecessary open mode parameters
+]


Is this still an accurate list? These linters rules seem useful, are we failing a lot of them?

So after looking at the code these are checks that I excluded for the following reasons, correct me if I am wrong:

B007: Sometimes we might want to have the variable unpacking to make clear what is being iterated over, no? As in for topic, name, items in group:, even if we just use topic/items?

E501: Would need quite a bit broken up or marked as noqa, as it also considers comments and long strings

E711: We use None to compare to sqlalchemy results - Correct me if I'm wrong, but I think these are not actually None, but just falsy?

E712: Same as above, just for True

F811: This currently applies to all fixture usages. We could fix this by not importing fixtures but having them in a conftest.py file

F841: This I assumed often refers to things that are in a variable that might be valuable later on, so I did not want to remove them all

UP015: Personal opinion, but I like to be excplicit about the open-mode

For each rule, you can comment out the ignore-line and check which lines would be affected to see for yourself as well. I think for F841 especially, you will be able to judge better what to exclude and what to leave.

To B007: You can put an underscore in front of the variable that is not required, but named. E.g: for topic, _name, items in group:.

@HansBambel Just saw that you commented, great to see you here 😄

.github/pull_request_template.md

spreeni · 2024-06-10T15:47:57Z

These are current files that are over 350kb - I guess most of these are fine, but maybe good to have an eye on it.

app/web/yarn.lock (658 KB) exceeds 350 KB.
docs/full_er_diagram.png (1585 KB) exceeds 350 KB.
app/web/features/dashboard/Hero/mesut-kaya-eOcyhe5-9sQ-unsplash.jpeg (3825 KB) exceeds 350 KB.
app/web/public/img/blog/20210407_lucas.jpg (484 KB) exceeds 350 KB.
app/web/public/img/blog/20210825_talkingtown.png (8739 KB) exceeds 350 KB.
app/backend/src/data/osm/80500.geojson (2582 KB) exceeds 350 KB.
app/web/public/img/blog/20210501_virtual_events.jpg (551 KB) exceeds 350 KB.
app/mobile/src/yarn.lock (747 KB) exceeds 350 KB.
app/backend/src/data/osm/62149.geojson (3206 KB) exceeds 350 KB.
app/backend/src/data/osm/61320.geojson (490 KB) exceeds 350 KB.
docs/current_er_diagram.png (1352 KB) exceeds 350 KB.
app/web/public/img/blog/20210604_mindy.jpg (439 KB) exceeds 350 KB.
app/backend/src/data/osm/54224.geojson (1003 KB) exceeds 350 KB.
app/backend/src/data/osm/148838.geojson (11182 KB) exceeds 350 KB.

spreeni · 2024-06-10T15:50:06Z

At some point, we could also integrate clang-format and eslint into the hook. Then a new person could just code away and would always have fully linted code when it came to the CI/CD pipeline.

This formatted the files, but always fails with an error "No such file or directory", so I left it out for now.

-   repo: https://github.com/pre-commit/mirrors-clang-format
    rev: 'v18.1.6'
    hooks:
      - id: clang-format
        args: ['--style=file:app/proto/.clang-format', '-i', '*.proto']
        files: ^.*.proto

aapeliv

I think you addressed all my questions. Looks good to me now!

# Conflicts: # app/backend/requirements.txt

spreeni · 2024-06-11T08:38:35Z

@aapeliv I merged develop into the branch, ready to be merged once you approve it

aapeliv

Thanks, please merge!

aapeliv · 2024-06-11T19:47:27Z

I really like this... it's so friggin fast xD

* Switch to ruff and applied linting rules * Add pre-commit and apply hooks to all files; Added ruff configuration to media and client * Restrict ruff pre-commit hook to only the backend * Adjusted pull request checklist template * Fix set error * Implementing of code review findings; Fix of further lint findings * Remove check-json from pre-commit; Applied clang-format * Fixed requirements.txt

spreeni added 4 commits June 10, 2024 00:32

Switch to ruff and applied linting rules

81e7e7e

Add pre-commit and apply hooks to all files; Added ruff configuration…

2b93980

… to media and client

Restrict ruff pre-commit hook to only the backend

7a1dcc5

Adjusted pull request checklist template

8466a81

spreeni self-assigned this Jun 9, 2024

spreeni added 1.topic backend This issue relates to the python backend refactoring labels Jun 9, 2024

Fix set error

eff0ae1

vercel bot deployed to Preview June 9, 2024 23:44 View deployment

spreeni requested a review from aapeliv June 9, 2024 23:45

aapeliv reviewed Jun 10, 2024

View reviewed changes

.github/pull_request_template.md Show resolved Hide resolved

Implementing of code review findings; Fix of further lint findings

e75b494

vercel bot deployed to Preview June 10, 2024 09:18 View deployment

spreeni requested a review from aapeliv June 10, 2024 09:20

Remove check-json from pre-commit; Applied clang-format

0f78e5e

vercel bot deployed to Preview June 10, 2024 15:55 View deployment

aapeliv previously approved these changes Jun 10, 2024

View reviewed changes

Merge branch 'develop' into refactor/add-ruff

c337bee

# Conflicts: # app/backend/requirements.txt

spreeni dismissed aapeliv’s stale review via c337bee June 11, 2024 07:17

vercel bot deployed to Preview June 11, 2024 07:20 View deployment

Fixed requirements.txt

ab891a0

vercel bot deployed to Preview June 11, 2024 07:41 View deployment

aapeliv approved these changes Jun 11, 2024

View reviewed changes

spreeni merged commit 45fbe38 into develop Jun 11, 2024
2 checks passed

spreeni deleted the refactor/add-ruff branch June 11, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to ruff / Add pre-commit hook #4428

Switch to ruff / Add pre-commit hook #4428

spreeni commented Jun 9, 2024 •

edited

Loading

vercel bot commented Jun 9, 2024 •

edited

Loading

aapeliv left a comment

aapeliv Jun 10, 2024

spreeni Jun 10, 2024 •

edited

Loading

HansBambel Jun 12, 2024

spreeni Jun 17, 2024

spreeni commented Jun 10, 2024

spreeni commented Jun 10, 2024 •

edited

Loading

aapeliv left a comment

spreeni commented Jun 11, 2024

aapeliv left a comment

aapeliv commented Jun 11, 2024

Switch to ruff / Add pre-commit hook #4428

Switch to ruff / Add pre-commit hook #4428

Conversation

spreeni commented Jun 9, 2024 • edited Loading

vercel bot commented Jun 9, 2024 • edited Loading

aapeliv left a comment

Choose a reason for hiding this comment

aapeliv Jun 10, 2024

Choose a reason for hiding this comment

spreeni Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

HansBambel Jun 12, 2024

Choose a reason for hiding this comment

spreeni Jun 17, 2024

Choose a reason for hiding this comment

spreeni commented Jun 10, 2024

spreeni commented Jun 10, 2024 • edited Loading

aapeliv left a comment

Choose a reason for hiding this comment

spreeni commented Jun 11, 2024

aapeliv left a comment

Choose a reason for hiding this comment

aapeliv commented Jun 11, 2024

spreeni commented Jun 9, 2024 •

edited

Loading

vercel bot commented Jun 9, 2024 •

edited

Loading

spreeni Jun 10, 2024 •

edited

Loading

spreeni commented Jun 10, 2024 •

edited

Loading