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.

Sign up for GitHub

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

Switch to ruff / Add pre-commit hook #4428

Merged
merged 9 commits into from
Jun 11, 2024
Merged

Switch to ruff / Add pre-commit hook #4428

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
81e7e7e
Switch to ruff and applied linting rules
spreeni Jun 9, 2024
2b93980
Add pre-commit and apply hooks to all files; Added ruff configuration…
spreeni Jun 9, 2024
7a1dcc5
Restrict ruff pre-commit hook to only the backend
spreeni Jun 9, 2024
8466a81
Adjusted pull request checklist template
spreeni Jun 9, 2024
eff0ae1
Fix set error
spreeni Jun 9, 2024
e75b494
Implementing of code review findings; Fix of further lint findings
spreeni Jun 10, 2024
0f78e5e
Remove check-json from pre-commit; Applied clang-format
spreeni Jun 10, 2024
c337bee
Merge branch 'develop' into refactor/add-ruff
spreeni Jun 11, 2024
ab891a0
Fixed requirements.txt
spreeni Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Checklists - you can remove one that is not applicable (ie. remove backend check
If you need help with any of these, please ask :)
--->
**Backend checklist**
- [ ] Formatted my code by running `autoflake --exclude src/proto -r -i --remove-all-unused-imports src && isort . && black .` in `app/backend`
- [ ] 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
Expand Down
1 change: 0 additions & 1 deletion app/.devcontainer/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,3 @@ services:

# Overrides default command so things don't shut down after the process ends.
command: /bin/sh -c "while sleep 1000; do :; done"

6 changes: 3 additions & 3 deletions app/.gitlab-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,9 @@ test:backend-format:
default: false
script:
- cd app/backend
- autoflake --exclude src/proto -r -i --remove-all-unused-imports --check src
- isort --check --diff .
- black --check --diff .
- ruff check .
- ruff check --diff .
- ruff format --diff .
rules:
- if: ($DO_CHECKS == "true") && ($CI_COMMIT_BRANCH == $RELEASE_BRANCH)
changes:
Expand Down
34 changes: 34 additions & 0 deletions app/backend/.pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
default_language_version:
python: python3.12

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.6.0
hooks:
- id: end-of-file-fixer
exclude: ^(.*.csv|.*.json|.*.svg)
- id: trailing-whitespace
- id: check-json
- id: check-toml
- id: check-yaml
- id: check-added-large-files
name: Check for added large files
description: Prevent giant files from being committed
entry: check-added-large-files
language: python
args: ['--maxkb=350', '--enforce-all']
- id: detect-private-key

- repo: https://github.com/charliermarsh/ruff-pre-commit
rev: 'v0.4.8'
hooks:
- id: ruff
description: Linter checks (without import sorting)
files: ^app/backend/
args: ['--ignore', 'I']
- id: ruff
files: ^app/backend/
args: ['--select', 'I', '--fix']
description: Sort imports
- id: ruff-format
files: ^app/backend/
31 changes: 24 additions & 7 deletions app/backend/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,29 @@
[tool.black]
[tool.ruff]
line-length = 120

[tool.isort]
skip_gitignore = true
include_trailing_comma = true
multi_line_output = 3
line_length = 120
known_first_party = ["couchers", "proto"]
[tool.ruff.lint]
select = [
"E", # pycodestyle
"F", # pyflakes
"UP", # pyupgrade
"B", # flake8-bugbear
"C4", # flake8-comprehensions
"A", # flake8-builtins
"I", # isort
]
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
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@spreeni spreeni Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


[tool.ruff.lint.isort]
known-first-party = ["couchers", "proto", "tests", "dummy_data"]
extra-standard-library = ["zoneinfo"]

[tool.coverage.run]
omit = ["src/proto/*", "src/couchers/migrations/*", "src/dummy_data.py"]
5 changes: 2 additions & 3 deletions app/backend/requirements.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
alembic
autoflake
black
boto3
cryptography
geoalchemy2
grpcio
http_ece
isort
Jinja2
luhn
phonenumbers
pre-commit
prometheus-client
protobuf
psycopg2-binary
Expand All @@ -21,6 +19,7 @@ python-dateutil
pytz
pyyaml
requests
ruff
sentry-sdk
Shapely
sqlalchemy
Expand Down
41 changes: 21 additions & 20 deletions app/backend/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@
#
alembic==1.13.1
# via -r requirements.in
autoflake==2.3.1
# via -r requirements.in
black==24.4.2
# via -r requirements.in
boto3==1.34.117
# via -r requirements.in
botocore==1.34.117
Expand All @@ -24,31 +20,33 @@ cffi==1.16.0
# via
# cryptography
# pynacl
cfgv==3.4.0
# via pre-commit
charset-normalizer==3.3.2
# via requests
click==8.1.7
# via black
coverage[toml]==7.5.0
# via pytest-cov
cryptography==42.0.7
# via
# -r requirements.in
# http-ece
# py-vapid
distlib==0.3.8
# via virtualenv
filelock==3.14.0
# via virtualenv
geoalchemy2==0.15.1
# via -r requirements.in
greenlet==3.0.3
# via sqlalchemy
grpcio==1.64.0
# via -r requirements.in
http-ece==1.2.0
# via -r requirements.in
identify==2.5.36
# via pre-commit
idna==3.7
# via requests
iniconfig==2.0.0
# via pytest
isort==5.13.2
# via -r requirements.in
jinja2==3.1.4
# via -r requirements.in
jmespath==1.0.1
Expand All @@ -63,23 +61,22 @@ markupsafe==2.1.5
# via
# jinja2
# mako
mypy-extensions==1.0.0
# via black
nodeenv==1.9.1
# via pre-commit
numpy==1.26.4
# via shapely
packaging==24.0
# via
# black
# geoalchemy2
# pytest
pathspec==0.12.1
# via black
phonenumbers==8.13.37
# via -r requirements.in
platformdirs==4.2.1
# via black
platformdirs==4.2.2
# via virtualenv
pluggy==1.5.0
# via pytest
pre-commit==3.7.1
# via -r requirements.in
prometheus-client==0.20.0
# via -r requirements.in
protobuf==5.27.0
Expand All @@ -90,8 +87,6 @@ py-vapid==1.9.1
# via -r requirements.in
pycparser==2.22
# via cffi
pyflakes==3.2.0
# via autoflake
pynacl==1.5.0
# via -r requirements.in
pytest==8.2.1
Expand All @@ -107,11 +102,15 @@ python-dateutil==2.9.0.post0
pytz==2024.1
# via -r requirements.in
pyyaml==6.0.1
# via -r requirements.in
# via
# -r requirements.in
# pre-commit
requests==2.32.3
# via
# -r requirements.in
# stripe
ruff==0.4.8
# via -r requirements.in
s3transfer==0.10.1
# via boto3
sentry-sdk==2.3.1
Expand Down Expand Up @@ -140,3 +139,5 @@ urllib3==2.2.1
# botocore
# requests
# sentry-sdk
virtualenv==20.26.2
# via pre-commit
11 changes: 5 additions & 6 deletions app/backend/src/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
import sys

import sentry_sdk
from sentry_sdk.integrations import argv, atexit, dedupe
from sentry_sdk.integrations import argv, atexit, dedupe, modules, stdlib, threading
from sentry_sdk.integrations import logging as sentry_logging
from sentry_sdk.integrations import modules, stdlib, threading
from sqlalchemy.sql import text

from couchers.config import check_config, config
Expand Down Expand Up @@ -58,28 +57,28 @@ def log_unhandled_exception(exc_type, exc_value, exc_traceback):

sys.excepthook = log_unhandled_exception

logger.info(f"Checking DB connection")
logger.info("Checking DB connection")

with session_scope() as session:
res = session.execute(text("SELECT 42;"))
if list(res) != [(42,)]:
raise Exception("Failed to connect to DB")

logger.info(f"Running DB migrations")
logger.info("Running DB migrations")

apply_migrations()

if config["ADD_DUMMY_DATA"]:
add_dummy_data()

logger.info(f"Starting")
logger.info("Starting")

if config["ROLE"] in ["api", "all"]:
server = create_main_server(port=1751)
server.start()
media_server = create_media_server(port=1753)
media_server.start()
logger.info(f"Serving on 1751 (secure) and 1753 (media)")
logger.info("Serving on 1751 (secure) and 1753 (media)")

if config["ROLE"] in ["scheduler", "all"]:
scheduler = start_jobs_scheduler()
Expand Down
6 changes: 4 additions & 2 deletions app/backend/src/couchers/email/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ def queue_email(
)


def enqueue_email_from_template(recipient, template_file, template_args={}, _footer_unsub_link=None):
def enqueue_email_from_template(recipient, template_file, template_args=None, _footer_unsub_link=None):
template_args = template_args or {}
frontmatter, plain, html = _render_email(template_file, template_args, _footer_unsub_link=_footer_unsub_link)
queue_email(
config["NOTIFICATION_EMAIL_SENDER"],
Expand All @@ -116,7 +117,8 @@ def enqueue_email_from_template(recipient, template_file, template_args={}, _foo
)


def enqueue_email_from_template_to_user(user, template_file, template_args={}, is_critical_email=False):
def enqueue_email_from_template_to_user(user, template_file, template_args=None, is_critical_email=False):
template_args = template_args or {}
if user.do_not_email and not is_critical_email:
logger.info(f"Not emailing {user} based on template {template_file} due to emails turned off")
return
Expand Down
4 changes: 2 additions & 2 deletions app/backend/src/couchers/helpers/clusters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def create_cluster(
admin_ids: List,
is_community: bool,
):
type = "community" if is_community else "group"
cluster_type = "community" if is_community else "group"
cluster = Cluster(
name=name,
description=description,
Expand All @@ -45,7 +45,7 @@ def create_cluster(
page_version = PageVersion(
page=main_page,
editor_user_id=creator_user_id,
title=DEFAULT_PAGE_TITLE_TEMPLATE.format(name=name, type=type),
title=DEFAULT_PAGE_TITLE_TEMPLATE.format(name=name, type=cluster_type),
content=DEFAULT_PAGE_CONTENT,
)
session.add(page_version)
Expand Down
2 changes: 1 addition & 1 deletion app/backend/src/couchers/interceptors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ def sanitizing_function(req, context):
# the code is one of the RPC error codes if this was failed through abort(), otherwise it's None
if not code:
logger.exception(e)
logger.info(f"Probably an unknown error! Sanitizing...")
logger.info("Probably an unknown error! Sanitizing...")
context.abort(grpc.StatusCode.INTERNAL, errors.UNKNOWN_ERROR)
else:
logger.warning(f"RPC error: {code}")
Expand Down
Loading