From 1fccd940ab687108d5bb29474f471211aec16e8b Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Mon, 28 Apr 2025 08:51:54 +0100 Subject: [PATCH 1/7] Reduce dependency on Klass instance in KlassDetailView This is a step on the road of removing the ORM from the template render. --- cbv/templates/cbv/klass_detail.html | 30 ++++++++++++++--------------- cbv/views.py | 20 +++++++++++++++++++ 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/cbv/templates/cbv/klass_detail.html b/cbv/templates/cbv/klass_detail.html index d290f436..7db5eace 100644 --- a/cbv/templates/cbv/klass_detail.html +++ b/cbv/templates/cbv/klass_detail.html @@ -5,13 +5,13 @@ {% load static %} -{% block title %}{{ klass.name }}{% endblock %} +{% block title %}{{ class.name }}{% endblock %} {% block meta_description %} - {{ klass.name }} in {{ project }}. - {% if klass.docstring %} - {{ klass.docstring }} + {{ class.name }} in {{ project }}. + {% if class.docstring %} + {{ class.docstring }} {% endif %} {% endblock meta_description %} @@ -40,8 +40,8 @@ {% block page_header %} -

class {{ klass.name }}

- 
from {{ klass.import_path }} import {{ klass.name }}
+

class {{ class.name }}

+ 
from {{ class.import_path }} import {{ class.name }}
{% with url=yuml_url %} {% if url %} @@ -50,15 +50,15 @@

class {{ klass.name }}

{% trans "Hierarchy diagram" %} {% endif %} {% endwith %} - {% if klass.docs_url %} - {% trans "Documentation" %} + {% if class.docs_url %} + {% trans "Documentation" %} {% else %} {% trans "Documentation" %} {% endif %} - {% trans "Source code" %} + {% trans "Source code" %}
- {% if klass.docstring %} - 
{{ klass.docstring }}
+ {% if class.docstring %} + 
{{ class.docstring }}
{% endif %} {% endblock %} @@ -70,7 +70,7 @@

class {{ klass.name }}

Ancestors (MRO)

    -
  1. {{ klass.name }}
    2. +
  2. {{ class.name }}
    3. {% for ancestor in all_ancestors %}
  3. @@ -115,8 +115,8 @@

    Attributes

    - {% if attribute.klass == klass %} - {{ attribute.klass.name }} + {% if attribute.klass_id == class.db_id %} + {{ class.name }} {% else %}     {{ attribute.klass.name }} {% endif %} @@ -151,7 +151,7 @@

    {% if namesakes|length == 1 %} {{ method.klass.name }} {% endif %} - +

    diff --git a/cbv/views.py b/cbv/views.py index 3497c057..5b5ae512 100644 --- a/cbv/views.py +++ b/cbv/views.py @@ -21,6 +21,16 @@ def get_redirect_url(self, *, url_name: str, **kwargs): class KlassDetailView(TemplateView): template_name = "cbv/klass_detail.html" + @attrs.frozen + class Class: + db_id: int + docs_url: str + docstring: str | None + import_path: str + name: str + source_url: str + url: str + @attrs.frozen class Ancestor: name: str @@ -56,6 +66,15 @@ def get_context_data(self, **kwargs): nav = nav_builder.get_nav_data( klass.module.project_version, klass.module, klass ) + class_data = self.Class( + db_id=klass.id, + name=klass.name, + docstring=klass.docstring, + docs_url=klass.docs_url, + import_path=klass.import_path, + source_url=klass.get_source_url(), + url=klass.get_absolute_url(), + ) direct_ancestors = list(klass.get_ancestors()) ancestors = [ self.Ancestor( @@ -77,6 +96,7 @@ def get_context_data(self, **kwargs): "all_children": children, "attributes": klass.get_prepared_attributes(), "canonical_url": self.request.build_absolute_uri(canonical_url_path), + "class": class_data, "klass": klass, "methods": list(klass.get_methods()), "nav": nav, From b5b45e978786a9092624f25c4ef6a1384f81eba3 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Thu, 1 May 2025 13:23:08 +0100 Subject: [PATCH 2/7] Move method ORM query and objects out of klass detail template We had a filter that did a complex query at template render time. That logic has now been moved to the view. Flat objects are introduced to passed method to the template instead. The helper function is horrible. We should also improve the method context data structure, but that would require more work on the template, so it's out of scope. Similarly, this makes no effort to add types to the helper function which was previously a template filter. --- cbv/templates/cbv/klass_detail.html | 15 ++++---- cbv/templatetags/cbv_tags.py | 26 -------------- cbv/views.py | 53 +++++++++++++++++++++++++++-- mypy-ratchet.json | 6 ++-- 4 files changed, 59 insertions(+), 41 deletions(-) delete mode 100644 cbv/templatetags/cbv_tags.py diff --git a/cbv/templates/cbv/klass_detail.html b/cbv/templates/cbv/klass_detail.html index 7db5eace..3c2bde11 100644 --- a/cbv/templates/cbv/klass_detail.html +++ b/cbv/templates/cbv/klass_detail.html @@ -1,6 +1,5 @@ {% extends "base.html" %} {% load pygmy %} -{% load cbv_tags %} {% load i18n %} {% load static %} @@ -140,7 +139,6 @@

    Attributes

    Methods

    {% endif %} {% ifchanged method.name %} - {% with namesakes=klass|namesake_methods:method.name %}

    @@ -148,20 +146,20 @@

    def {{ method.name }}({{ method.kwargs }}): - {% if namesakes|length == 1 %} - {{ method.klass.name }} + {% if method.namesakes|length == 1 %} + {{ method.namesakes.0.class_name }} {% endif %}

    - {% for namesake in namesakes %} - {% if namesakes|length != 1 %} + {% for namesake in method.namesakes %} + {% if method.namesakes|length != 1 %}
    -

    {{ namesake.klass.name }}

    +

    {{ namesake.class_name }}

     -
    +
    {% if namesake.docstring %}
    {{ namesake.docstring }}
    {% endif %} {% pygmy namesake.code linenos='True' linenostart=namesake.line_number lexer='python' %} @@ -175,7 +173,6 @@

    {{ namesake.klass.name }}

    {% endfor %}
    - {% endwith %} {% endifchanged %} {% if forloop.last %}
    {% endif %} {% endfor %} diff --git a/cbv/templatetags/cbv_tags.py b/cbv/templatetags/cbv_tags.py deleted file mode 100644 index a727d38d..00000000 --- a/cbv/templatetags/cbv_tags.py +++ /dev/null @@ -1,26 +0,0 @@ -from django import template - - -register = template.Library() - - -@register.filter -def namesake_methods(parent_klass, name): - namesakes = [m for m in parent_klass.get_methods() if m.name == name] - assert namesakes - # Get the methods in order of the klasses - try: - result = [next(m for m in namesakes if m.klass == parent_klass)] - namesakes.pop(namesakes.index(result[0])) - except StopIteration: - result = [] - for klass in parent_klass.get_all_ancestors(): - # Move the namesakes from the methods to the results - try: - method = next(m for m in namesakes if m.klass == klass) - namesakes.pop(namesakes.index(method)) - result.append(method) - except StopIteration: - pass - assert not namesakes - return result diff --git a/cbv/views.py b/cbv/views.py index 5b5ae512..ccde271a 100644 --- a/cbv/views.py +++ b/cbv/views.py @@ -1,3 +1,4 @@ +from collections.abc import Sequence from typing import Any import attrs @@ -42,6 +43,19 @@ class Child: name: str url: str + @attrs.frozen + class Method: + @attrs.frozen + class MethodInstance: + docstring: str + code: str + line_number: int + class_name: str + + name: str + kwargs: str + namesakes: Sequence[MethodInstance] + def get_context_data(self, **kwargs): qs = Klass.objects.filter( name__iexact=self.kwargs["klass"], @@ -91,14 +105,29 @@ def get_context_data(self, **kwargs): ) for child in klass.get_all_children() ] + methods = [ + self.Method( + name=method.name, + kwargs=method.kwargs, + namesakes=[ + self.Method.MethodInstance( + docstring=method_instance.docstring, + code=method_instance.code, + line_number=method_instance.line_number, + class_name=method_instance.klass.name, + ) + for method_instance in self._namesake_methods(klass, method.name) + ], + ) + for method in klass.get_methods() + ] return { "all_ancestors": ancestors, "all_children": children, "attributes": klass.get_prepared_attributes(), "canonical_url": self.request.build_absolute_uri(canonical_url_path), "class": class_data, - "klass": klass, - "methods": list(klass.get_methods()), + "methods": methods, "nav": nav, "project": f"Django {klass.module.project_version.version_number}", "push_state_url": push_state_url, @@ -106,6 +135,26 @@ def get_context_data(self, **kwargs): "yuml_url": klass.basic_yuml_url(), } + def _namesake_methods(self, parent_klass, name): + namesakes = [m for m in parent_klass.get_methods() if m.name == name] + assert namesakes + # Get the methods in order of the klasses + try: + result = [next(m for m in namesakes if m.klass == parent_klass)] + namesakes.pop(namesakes.index(result[0])) + except StopIteration: + result = [] + for klass in parent_klass.get_all_ancestors(): + # Move the namesakes from the methods to the results + try: + method = next(m for m in namesakes if m.klass == klass) + namesakes.pop(namesakes.index(method)) + result.append(method) + except StopIteration: + pass + assert not namesakes + return result + class LatestKlassRedirectView(RedirectView): def get_redirect_url(self, **kwargs): diff --git a/mypy-ratchet.json b/mypy-ratchet.json index 55ea6bcd..16ae4776 100644 --- a/mypy-ratchet.json +++ b/mypy-ratchet.json @@ -25,15 +25,13 @@ "\"Callable[[Module], tuple[str, str, str]]\" has no attribute \"dependencies\" [attr-defined]": 1, "Missing return statement [return]": 1 }, - "cbv/templatetags/cbv_tags.py": { - "Function is missing a type annotation [no-untyped-def]": 1 - }, "cbv/views.py": { + "Call to untyped function \"_namesake_methods\" in typed context [no-untyped-call]": 1, "Call to untyped function \"get_fuzzy_object\" in typed context [no-untyped-call]": 1, "Call to untyped function \"get_object\" in typed context [no-untyped-call]": 1, "Call to untyped function \"get_precise_object\" in typed context [no-untyped-call]": 1, "Function is missing a return type annotation [no-untyped-def]": 1, - "Function is missing a type annotation [no-untyped-def]": 9, + "Function is missing a type annotation [no-untyped-def]": 10, "Function is missing a type annotation for one or more arguments [no-untyped-def]": 1 } } From baa95067163829afac1c73c7ce3930d9f958932f Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Thu, 1 May 2025 13:49:42 +0100 Subject: [PATCH 3/7] Move attributes query off model, onto view This adds a `klass` argument, and renames a couple of attributes. Other than that, this is just a move. This function is gnarly. Fixing it can come later. --- cbv/models.py | 34 ---------------------------------- cbv/views.py | 39 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/cbv/models.py b/cbv/models.py index dddae0d3..c1818d9b 100644 --- a/cbv/models.py +++ b/cbv/models.py @@ -257,40 +257,6 @@ def get_attributes(self) -> models.QuerySet["KlassAttribute"]: self._attributes = attrs return self._attributes - def get_prepared_attributes(self) -> models.QuerySet["KlassAttribute"]: - attributes = self.get_attributes() - # Make a dictionary of attributes based on name - attribute_names: dict[str, list[KlassAttribute]] = {} - for attr in attributes: - try: - attribute_names[attr.name] += [attr] - except KeyError: - attribute_names[attr.name] = [attr] - - ancestors = self.get_all_ancestors() - - # Find overridden attributes - for name, attrs in attribute_names.items(): - # Skip if we have only one attribute. - if len(attrs) == 1: - continue - - # Sort the attributes by ancestors. - def _key(a: KlassAttribute) -> int: - try: - # If ancestor, return the index (>= 0) - return ancestors.index(a.klass) - except ValueError: # Raised by .index if item is not in list. - # else a.klass == self, so return -1 - return -1 - - sorted_attrs = sorted(attrs, key=_key) - - # Mark overriden KlassAttributes - for a in sorted_attrs[1:]: - a.overridden = True - return attributes - def basic_yuml_data(self, first: bool = False) -> list[str]: self._basic_yuml_data: list[str] if hasattr(self, "_basic_yuml_data"): diff --git a/cbv/views.py b/cbv/views.py index ccde271a..77c256a0 100644 --- a/cbv/views.py +++ b/cbv/views.py @@ -3,10 +3,11 @@ import attrs from django import http +from django.db.models import QuerySet from django.urls import reverse from django.views.generic import RedirectView, TemplateView, View -from cbv.models import Klass, Module, ProjectVersion +from cbv.models import Klass, KlassAttribute, Module, ProjectVersion from cbv.queries import NavBuilder @@ -124,7 +125,7 @@ def get_context_data(self, **kwargs): return { "all_ancestors": ancestors, "all_children": children, - "attributes": klass.get_prepared_attributes(), + "attributes": self._get_prepared_attributes(klass), "canonical_url": self.request.build_absolute_uri(canonical_url_path), "class": class_data, "methods": methods, @@ -155,6 +156,40 @@ def _namesake_methods(self, parent_klass, name): assert not namesakes return result + def _get_prepared_attributes(self, klass: Klass) -> QuerySet["KlassAttribute"]: + attributes = klass.get_attributes() + # Make a dictionary of attributes based on name + attribute_names: dict[str, list[KlassAttribute]] = {} + for attr in attributes: + try: + attribute_names[attr.name] += [attr] + except KeyError: + attribute_names[attr.name] = [attr] + + ancestors = klass.get_all_ancestors() + + # Find overridden attributes + for name, klass_attributes in attribute_names.items(): + # Skip if we have only one attribute. + if len(klass_attributes) == 1: + continue + + # Sort the attributes by ancestors. + def _key(a: KlassAttribute) -> int: + try: + # If ancestor, return the index (>= 0) + return ancestors.index(a.klass) + except ValueError: # Raised by .index if item is not in list. + # else a.klass == klass, so return -1 + return -1 + + sorted_attrs = sorted(klass_attributes, key=_key) + + # Mark overriden KlassAttributes + for a in sorted_attrs[1:]: + a.overridden = True + return attributes + class LatestKlassRedirectView(RedirectView): def get_redirect_url(self, **kwargs): From ae0955a57947bfa7b1bc10ef0287007a50fabfb9 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Thu, 1 May 2025 14:17:53 +0100 Subject: [PATCH 4/7] Reduce boilerplate by using defaultdict --- cbv/views.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cbv/views.py b/cbv/views.py index 77c256a0..195c56c3 100644 --- a/cbv/views.py +++ b/cbv/views.py @@ -1,3 +1,4 @@ +from collections import defaultdict from collections.abc import Sequence from typing import Any @@ -159,12 +160,9 @@ def _namesake_methods(self, parent_klass, name): def _get_prepared_attributes(self, klass: Klass) -> QuerySet["KlassAttribute"]: attributes = klass.get_attributes() # Make a dictionary of attributes based on name - attribute_names: dict[str, list[KlassAttribute]] = {} + attribute_names: dict[str, list[KlassAttribute]] = defaultdict(list) for attr in attributes: - try: - attribute_names[attr.name] += [attr] - except KeyError: - attribute_names[attr.name] = [attr] + attribute_names[attr.name].append(attr) ancestors = klass.get_all_ancestors() From 478666a9fd53f0441a966270886c9ee059985912 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Thu, 1 May 2025 14:26:49 +0100 Subject: [PATCH 5/7] Move helper function out of loop There's no need for us to re-define this on every iteration. --- cbv/views.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/cbv/views.py b/cbv/views.py index 195c56c3..885469a3 100644 --- a/cbv/views.py +++ b/cbv/views.py @@ -166,21 +166,21 @@ def _get_prepared_attributes(self, klass: Klass) -> QuerySet["KlassAttribute"]: ancestors = klass.get_all_ancestors() + # Sort the attributes by ancestors. + def _key(a: KlassAttribute) -> int: + try: + # If ancestor, return the index (>= 0) + return ancestors.index(a.klass) + except ValueError: # Raised by .index if item is not in list. + # else a.klass == klass, so return -1 + return -1 + # Find overridden attributes for name, klass_attributes in attribute_names.items(): # Skip if we have only one attribute. if len(klass_attributes) == 1: continue - # Sort the attributes by ancestors. - def _key(a: KlassAttribute) -> int: - try: - # If ancestor, return the index (>= 0) - return ancestors.index(a.klass) - except ValueError: # Raised by .index if item is not in list. - # else a.klass == klass, so return -1 - return -1 - sorted_attrs = sorted(klass_attributes, key=_key) # Mark overriden KlassAttributes From d38f4a40eb23b21e5154db6582e2a53d020bc872 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Thu, 1 May 2025 14:27:20 +0100 Subject: [PATCH 6/7] Avoid fetching unused dictionary key --- cbv/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cbv/views.py b/cbv/views.py index 885469a3..04bfbe1d 100644 --- a/cbv/views.py +++ b/cbv/views.py @@ -176,7 +176,7 @@ def _key(a: KlassAttribute) -> int: return -1 # Find overridden attributes - for name, klass_attributes in attribute_names.items(): + for klass_attributes in attribute_names.values(): # Skip if we have only one attribute. if len(klass_attributes) == 1: continue From a24b7e2311c6b2a0a02c607d1b151230b192dc32 Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Fri, 2 May 2025 17:17:01 +0100 Subject: [PATCH 7/7] Remove Attribute ORM models from template render --- cbv/templates/cbv/klass_detail.html | 4 ++-- cbv/views.py | 24 +++++++++++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/cbv/templates/cbv/klass_detail.html b/cbv/templates/cbv/klass_detail.html index 3c2bde11..9e13cbde 100644 --- a/cbv/templates/cbv/klass_detail.html +++ b/cbv/templates/cbv/klass_detail.html @@ -114,10 +114,10 @@

    Attributes

    - {% if attribute.klass_id == class.db_id %} + {% if not attribute.class_url %} {{ class.name }} {% else %} - {{ attribute.klass.name }} + {{ attribute.class_name }} {% endif %} diff --git a/cbv/views.py b/cbv/views.py index 04bfbe1d..19b46b64 100644 --- a/cbv/views.py +++ b/cbv/views.py @@ -58,6 +58,14 @@ class MethodInstance: kwargs: str namesakes: Sequence[MethodInstance] + @attrs.frozen + class Attribute: + name: str + value: str + overridden: bool + class_url: str | None + class_name: str + def get_context_data(self, **kwargs): qs = Klass.objects.filter( name__iexact=self.kwargs["klass"], @@ -123,10 +131,24 @@ def get_context_data(self, **kwargs): ) for method in klass.get_methods() ] + attributes = [ + self.Attribute( + name=attribute.name, + value=attribute.value, + overridden=hasattr(attribute, "overridden"), + class_url=( + attribute.klass.get_absolute_url() + if attribute.klass_id != klass.id + else None + ), + class_name=attribute.klass.name, + ) + for attribute in self._get_prepared_attributes(klass) + ] return { "all_ancestors": ancestors, "all_children": children, - "attributes": self._get_prepared_attributes(klass), + "attributes": attributes, "canonical_url": self.request.build_absolute_uri(canonical_url_path), "class": class_data, "methods": methods,