From 675c0e5ac6a6f297d35d563f06f9da80471524a6 Mon Sep 17 00:00:00 2001 From: Restioson Date: Thu, 7 Nov 2024 12:27:52 +0200 Subject: [PATCH] lint: include many more lints By changing `select` to `extend-select`, we keep the default Ruff lints enabled. Additionally, there are some more lints that we care about that have both been enabled and fixed in this commit. --- app/accounts/admin.py | 2 -- app/accounts/models.py | 2 -- app/accounts/views.py | 1 - app/app/urls.py | 1 - app/general/filters.py | 4 +-- .../management/commands/import_documents.py | 2 +- app/general/tests/test_document_detail.py | 2 +- app/general/tests/test_filter.py | 32 +++++++++---------- app/general/tests/test_frontend.py | 30 ++++++++++------- app/general/tests/test_import_documents.py | 10 +++--- app/general/tests/test_project_detail.py | 2 +- app/general/tests/test_view_search.py | 2 +- app/general/tests/test_views_institution.py | 2 +- app/pyproject.toml | 11 ++++--- 14 files changed, 54 insertions(+), 49 deletions(-) diff --git a/app/accounts/admin.py b/app/accounts/admin.py index 8c38f3f3..846f6b40 100644 --- a/app/accounts/admin.py +++ b/app/accounts/admin.py @@ -1,3 +1 @@ -from django.contrib import admin - # Register your models here. diff --git a/app/accounts/models.py b/app/accounts/models.py index 71a83623..6b202199 100644 --- a/app/accounts/models.py +++ b/app/accounts/models.py @@ -1,3 +1 @@ -from django.db import models - # Create your models here. diff --git a/app/accounts/views.py b/app/accounts/views.py index 375e0135..15e2e5b1 100644 --- a/app/accounts/views.py +++ b/app/accounts/views.py @@ -1,4 +1,3 @@ -from django.contrib.auth import authenticate from django.contrib.auth import login as auth_login from django.contrib.auth.views import LoginView as _LoginView from django.contrib.auth.views import PasswordChangeView as _PasswordChangeView diff --git a/app/app/urls.py b/app/app/urls.py index afd11b25..6cc6f633 100644 --- a/app/app/urls.py +++ b/app/app/urls.py @@ -18,7 +18,6 @@ from django.conf import settings from django.conf.urls.static import static from django.contrib import admin -from django.contrib.auth import views as auth_views from django.urls import include, path from django.utils.translation import gettext_lazy as _ diff --git a/app/general/filters.py b/app/general/filters.py index 0eaa4882..fbd38c32 100644 --- a/app/general/filters.py +++ b/app/general/filters.py @@ -7,10 +7,10 @@ SearchVector, ) from django.db.models import F, Value -from django.db.models.functions import Greatest, Left +from django.db.models.functions import Left from django.db.models.query import EmptyQuerySet from django.utils.translation import gettext_lazy as _ -from django_filters import ModelMultipleChoiceFilter, MultipleChoiceFilter +from django_filters import ModelMultipleChoiceFilter from general.models import Document, Institution, Language, Project, Subject diff --git a/app/general/management/commands/import_documents.py b/app/general/management/commands/import_documents.py index b4e2042d..bc9bad4b 100644 --- a/app/general/management/commands/import_documents.py +++ b/app/general/management/commands/import_documents.py @@ -47,7 +47,7 @@ def save_data(self, file_path, file_name): document_data=pdf_to_text(file_path), uploaded_file=content_file, document_type="Glossary", - institution_id=random.randint(1, 20), + institution_id=random.randint(1, 20), # noqa: S311 - this is a fixture; security isn't important ) instance.save() instance.languages.set(Language.objects.all()) diff --git a/app/general/tests/test_document_detail.py b/app/general/tests/test_document_detail.py index d6ea0c1b..b0e5c363 100644 --- a/app/general/tests/test_document_detail.py +++ b/app/general/tests/test_document_detail.py @@ -1,7 +1,7 @@ from django.test import TestCase from django.urls import reverse -from general.models import Document, Institution, Language, Project, Subject +from general.models import Document, Institution, Language, Subject class DocumentDetailViewTest(TestCase): diff --git a/app/general/tests/test_filter.py b/app/general/tests/test_filter.py index 36f36190..49b84e01 100644 --- a/app/general/tests/test_filter.py +++ b/app/general/tests/test_filter.py @@ -48,23 +48,23 @@ def setUp(self): def test_institution_filter(self): data = {"institution": [self.institution1.id]} - filter = DocumentFilter(data=data) - qs = filter.qs + doc_filter = DocumentFilter(data=data) + qs = doc_filter.qs self.assertEqual(len(qs), 2) # TODO: ordering between documents and projects are not yet defined self.assertEqual(qs[0]["id"], self.project1.id) def test_subjects_filter(self): data = {"subjects": [self.subject1.id]} - filter = DocumentFilter(data=data) - qs = filter.qs + doc_filter = DocumentFilter(data=data) + qs = doc_filter.qs self.assertEqual(len(qs), 1) self.assertEqual(qs[0]["id"], self.doc1.id) def test_languages_filter(self): data = {"languages": [self.language1.id]} - filter = DocumentFilter(data=data) - qs = filter.qs + doc_filter = DocumentFilter(data=data) + qs = doc_filter.qs self.assertEqual(len(qs), 1) self.assertEqual(qs[0]["id"], self.doc1.id) @@ -74,35 +74,35 @@ def test_combined_filters(self): "subjects": [self.subject1.id], "languages": [self.language1.id], } - filter = DocumentFilter(data=data) - qs = filter.qs + doc_filter = DocumentFilter(data=data) + qs = doc_filter.qs self.assertEqual(len(qs), 1) self.assertEqual(qs[0]["id"], self.doc1.id) def test_search_filter_documents(self): data = {"search": "Document"} - filter = DocumentFilter(data=data) - qs = filter.qs + doc_filter = DocumentFilter(data=data) + qs = doc_filter.qs self.assertEqual(len(qs), 2) self.assertCountEqual([qs[0]["id"], qs[1]["id"]], [self.doc1.id, self.doc2.id]) data = {"search": "Document 1"} - filter = DocumentFilter(data=data) - qs = filter.qs + doc_filter = DocumentFilter(data=data) + qs = doc_filter.qs self.assertEqual(len(qs), 1) self.assertEqual(qs[0]["id"], self.doc1.id) def test_search_filter_projects(self): data = {"search": "Project 1"} - filter = DocumentFilter(data=data) - qs = filter.qs + doc_filter = DocumentFilter(data=data) + qs = doc_filter.qs self.assertEqual(len(qs), 1) self.assertEqual(qs[0]["id"], self.project1.id) def test_search_filter_combined(self): data = {"search": "1"} - filter = DocumentFilter(data=data) - qs_ids = [x["id"] for x in filter.qs] + doc_filter = DocumentFilter(data=data) + qs_ids = [x["id"] for x in doc_filter.qs] expected_ids = [self.doc1.id, self.project1.id, self.institution1.id] self.assertCountEqual(qs_ids, expected_ids) # TODO: test properly instead of relying on randomly agreeing IDs diff --git a/app/general/tests/test_frontend.py b/app/general/tests/test_frontend.py index 97779cd2..867cd2a3 100644 --- a/app/general/tests/test_frontend.py +++ b/app/general/tests/test_frontend.py @@ -29,9 +29,10 @@ def test_no_404s(self): # Sanity check in case we ever change the 404 title self.driver.get(f"{self.live_server_url}/blabla404") print(self.driver.title) - assert self.driver.title.startswith( - "Error" - ), f"Actual title was {self.driver.title}. Page: {self.driver.page_source}" + self.assertTrue( + self.driver.title.startswith("Error"), + f"Actual title was {self.driver.title}. Page: {self.driver.page_source}", + ) # Check main page does not 404 self.driver.get(self.live_server_url) @@ -49,15 +50,22 @@ def check_nav_item(self, link_text): wait = WebDriverWait(self.driver, timeout=WAIT_TIMEOUT) wait.until(lambda d: link_text in self.driver.title) except TimeoutException: - assert link_text in self.driver.title, ( - f"Expected title for page {link_text} to have {link_text};" - f" was {self.driver.title}" + self.assertIn( + link_text, + self.driver.title, + ( + f"Expected title for page {link_text} to have {link_text};" + f" was {self.driver.title}" + ), ) self.assert_current_page_not_error() def assert_current_page_not_error(self): - assert not self.driver.title.startswith("Error"), f"Actual title was {self.driver.title}" - assert not self.driver.title.startswith( - "ProgrammingError" - ), f"Actual title was {self.driver.title}" - assert not self.driver.find_element(By.ID, "error-block").is_displayed() + self.assertFalse( + self.driver.title.startswith("Error"), f"Actual title was {self.driver.title}" + ) + self.assertFalse( + self.driver.title.startswith("ProgrammingError"), + f"Actual title was {self.driver.title}", + ) + self.assertFalse(self.driver.find_element(By.ID, "error-block").is_displayed()) diff --git a/app/general/tests/test_import_documents.py b/app/general/tests/test_import_documents.py index a18de80b..1925f5ce 100644 --- a/app/general/tests/test_import_documents.py +++ b/app/general/tests/test_import_documents.py @@ -41,13 +41,13 @@ def test_save_data(self): command = Command() # Create some Institutions instances for testing for i in range(1, 21): - id = random.randint(1, 1000) + institution_id = random.randint(1, 1000) # noqa: S311 - this is only for testing Institution.objects.create( id=i, - name=f"{id}_{self.fake.company()}", - abbreviation=f"{id}_{self.fake.company_suffix()}", - url=f"{id}_{self.fake.url()}", - email=f"{id}_{self.fake.company_email()}", + name=f"{institution_id}_{self.fake.company()}", + abbreviation=f"{institution_id}_{self.fake.company_suffix()}", + url=f"{institution_id}_{self.fake.url()}", + email=f"{institution_id}_{self.fake.company_email()}", logo="", ) diff --git a/app/general/tests/test_project_detail.py b/app/general/tests/test_project_detail.py index 5991b237..4ba4f539 100644 --- a/app/general/tests/test_project_detail.py +++ b/app/general/tests/test_project_detail.py @@ -49,7 +49,7 @@ def test_project_detail_view_context(self): def test_project_detail_view_num_queries(self): with self.assertNumQueries(3): - response = self.client.get(reverse("project_detail", args=[self.project.id])) + self.client.get(reverse("project_detail", args=[self.project.id])) def test_project_detail_view_invalid_id(self): invalid_project_id = self.project.id + 1 diff --git a/app/general/tests/test_view_search.py b/app/general/tests/test_view_search.py index 2a3b7af8..56a45835 100644 --- a/app/general/tests/test_view_search.py +++ b/app/general/tests/test_view_search.py @@ -1,6 +1,6 @@ import unittest -from django.test import Client, TestCase +from django.test import TestCase from django.urls import reverse from general.models import Document, Institution, Language, Subject diff --git a/app/general/tests/test_views_institution.py b/app/general/tests/test_views_institution.py index 9690c56f..a39e30c0 100644 --- a/app/general/tests/test_views_institution.py +++ b/app/general/tests/test_views_institution.py @@ -1,4 +1,4 @@ -from django.test import Client, TestCase +from django.test import TestCase from django.urls import reverse from general.models import Document, Institution, Project diff --git a/app/pyproject.toml b/app/pyproject.toml index 5be1384c..49d7d6dc 100644 --- a/app/pyproject.toml +++ b/app/pyproject.toml @@ -3,8 +3,11 @@ line-length = 100 exclude = ["migrations", ".venv"] [tool.ruff.lint] -select = [ - "DJ", - "I", - "W" +extend-select = [ + "DJ", # Django - flake-8-django - https://docs.astral.sh/ruff/rules/#flake8-django-dj + "I", # Imports - isort - https://docs.astral.sh/ruff/rules/#isort-i + "W", # General - pycodestyle - https://docs.astral.sh/ruff/rules/#pycodestyle-e-w + "S", # Security - flake-8-bandit https://docs.astral.sh/ruff/rules/#flake8-bandit-s + "F", # General - Pyflakes - https://docs.astral.sh/ruff/rules/#pyflakes-f + "A" # Shadowing of builtins - flake-8-builtins - https://docs.astral.sh/ruff/rules/#flake8-builtins-a ]