-
Notifications
You must be signed in to change notification settings - Fork 73
Reduce dependency on Klass instance in KlassDetailView #279
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.
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
Open
meshy
wants to merge
7
commits into
main
Choose a base branch
from
drive-orm-from-templates
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
1fccd94
Reduce dependency on Klass instance in KlassDetailView
meshy b5b45e9
Move method ORM query and objects out of klass detail template
meshy baa9506
Move attributes query off model, onto view
meshy ae0955a
Reduce boilerplate by using defaultdict
meshy 478666a
Move helper function out of loop
meshy d38f4a4
Avoid fetching unused dictionary key
meshy a24b7e2
Remove Attribute ORM models from template render
meshy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,14 @@ | ||
| from collections import defaultdict | ||
| from collections.abc import Sequence | ||
| from typing import Any | ||
|
|
||
| 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 | ||
|
|
||
|
|
||
|
|
@@ -21,6 +24,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 | ||
|
|
@@ -32,6 +45,27 @@ 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] | ||
|
|
||
| @attrs.frozen | ||
| class Attribute: | ||
| name: str | ||
| value: str | ||
| overridden: bool | ||
| class_url: str | None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question here, about using empty string instead of None |
||
| class_name: str | ||
|
|
||
| def get_context_data(self, **kwargs): | ||
| qs = Klass.objects.filter( | ||
| name__iexact=self.kwargs["klass"], | ||
|
|
@@ -56,6 +90,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( | ||
|
|
@@ -72,20 +115,101 @@ 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() | ||
| ] | ||
| 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": klass.get_prepared_attributes(), | ||
| "attributes": attributes, | ||
| "canonical_url": self.request.build_absolute_uri(canonical_url_path), | ||
| "klass": klass, | ||
| "methods": list(klass.get_methods()), | ||
| "class": class_data, | ||
| "methods": methods, | ||
| "nav": nav, | ||
| "project": f"Django {klass.module.project_version.version_number}", | ||
| "push_state_url": push_state_url, | ||
| "version_switcher": version_switcher, | ||
| "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 | ||
|
|
||
| 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]] = defaultdict(list) | ||
| for attr in attributes: | ||
| attribute_names[attr.name].append(attr) | ||
|
|
||
| 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 klass_attributes in attribute_names.values(): | ||
| # Skip if we have only one attribute. | ||
| if len(klass_attributes) == 1: | ||
| continue | ||
|
|
||
| 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): | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could an empty string serve the same falsey/emptiness need as None here, and reduce the number of types?