Merge pull request #146 from SADiLaR/lint/expanded-linting

Expanded linting

app/accounts/admin.py

@@ -1,3 +1 @@
-from django.contrib import admin
-
 # Register your models here.

app/accounts/models.py

@@ -1,3 +1 @@
-from django.db import models
-
 # Create your models here.

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

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 _
 

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
 

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())

app/general/templatetags/bs_icons.py

@@ -1,5 +1,3 @@
-import re
-
 from django import template
 from django.utils.safestring import mark_safe
 
@@ -12,20 +10,6 @@
 
 
 register = template.Library()
-icon_name_re = re.compile(r"[a-z0-9\-]+")
-
-
-def _bs_icon(name):
-    assert icon_name_re.fullmatch(name)
-    return mark_safe(f'<i aria-hidden="true" class="icon bi-{name}"></i> ')
-    # The trailing space is intentional: Since this is an inline element
-    # usually followed by text, the absence/presence of a space is significant,
-    # and usually wanted for layout. That's too hard to remember, so we always
-    # add it. Multiple spaces are equal to one. That way the exact layout of
-    # code in the templates doesn't matter. Beware of using {% spaceless %}
-    # which will negate this. A pure CSS solution escaped me thus far, since a
-    # space will take additional space in addition to a margin.
-
 
 # a mapping from project types to Bootstrap icon names:
 _icons = {
@@ -44,4 +28,14 @@ def icon(name):
     if not (bs_name := _icons.get(name)):
         raise template.TemplateSyntaxError(f"'icon' requires a registered icon name (got {name!r})")
 
-    return _bs_icon(bs_name)
+    # This `mark_safe` is okay because we only allow certain, whitelisted strings. This is enforced above by fetching it
+    # from the `_icons` dictionary
+    return mark_safe(f'<i aria-hidden="true" class="icon bi-{bs_name}"></i> ')  # noqa: S308 - see above
+
+    # The trailing space is intentional: Since this is an inline element
+    # usually followed by text, the absence/presence of a space is significant,
+    # and usually wanted for layout. That's too hard to remember, so we always
+    # add it. Multiple spaces are equal to one. That way the exact layout of
+    # code in the templates doesn't matter. Beware of using {% spaceless %}
+    # which will negate this. A pure CSS solution escaped me thus far, since a
+    # space will take additional space in addition to a margin.

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

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

app/general/tests/test_frontend.py

@@ -78,9 +78,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)
@@ -98,15 +99,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())

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="",
             )
 

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

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

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

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
 ]