-
-
Notifications
You must be signed in to change notification settings - Fork 131
Nested children past the first level #507
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
Changes from all commits
8dedaaf
1c29ec8
49e2ca5
5f63d4f
f1c9299
e5e01ad
79fbc01
3274d0e
5f8548c
185cf97
546d7d7
c299d1a
14beb71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,9 @@ venv.bak/ | |
| pip-wheel-metadata | ||
| TODO.md | ||
|
|
||
| # pycharm | ||
| .idea/ | ||
|
|
||
| node_modules/ | ||
| tags | ||
| staticfiles/ | ||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,33 +72,55 @@ def dicts_equal(dictionary_one: Dict, dictionary_two: Dict) -> bool: | |
| return is_valid | ||
|
|
||
|
|
||
| def get_cacheable_component( | ||
| component: "django_unicorn.views.UnicornView", | ||
| ) -> "django_unicorn.views.UnicornView": | ||
| class CacheableComponent: | ||
|
Contributor
Author
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. I'll be interested in what you think of this approach. I found having to create the cacheable component, then restore back the extra_context/request everywhere was scary. Meaning I could forget a spot and not notice. I would hope the enter/exit/with approach would help with that, but maybe its awkward.
Owner
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. This is a much better pattern and using a context manager totally makes sense for this use case. |
||
| """ | ||
| Converts a component into something that is cacheable/pickleable. | ||
| Updates a component into something that is cacheable/pickleable. Use in a `with` statement | ||
| or explicitly call `__enter__` `__exit__` to use. It will restore the original component | ||
| on exit. | ||
| """ | ||
|
|
||
| component.request = None | ||
|
|
||
| if component.extra_context: | ||
| component.extra_context = None | ||
|
|
||
| if component.parent: | ||
| component.parent = get_cacheable_component(component.parent) | ||
|
|
||
| for child in component.children: | ||
| if child.request is not None: | ||
| child = get_cacheable_component(child) | ||
|
|
||
| try: | ||
| pickle.dumps(component) | ||
| except (TypeError, AttributeError, NotImplementedError, pickle.PicklingError) as e: | ||
| raise UnicornCacheError( | ||
| f"Cannot cache component '{type(component)}' because it is not picklable: {type(e)}: {e}" | ||
| ) from e | ||
|
|
||
| return component | ||
| def __init__(self, component: "django_unicorn.views.UnicornView"): | ||
| self._state = {} | ||
| self.cacheable_component = component | ||
|
|
||
| def __enter__(self): | ||
| components = [] | ||
| components.append(self.cacheable_component) | ||
| while components: | ||
| component = components.pop() | ||
| if component.component_id in self._state: | ||
| continue | ||
| if hasattr(component, "extra_context"): | ||
| extra_context = component.extra_context | ||
| component.extra_context = None | ||
| else: | ||
| extra_context = None | ||
| request = component.request | ||
| component.request = None | ||
| self._state[component.component_id] = (component, request, extra_context) | ||
| if component.parent: | ||
| components.append(component.parent) | ||
| for child in component.children: | ||
| components.append(child) | ||
|
|
||
| for component, _, _ in self._state.values(): | ||
| try: | ||
| pickle.dumps(component) | ||
| except ( | ||
| TypeError, | ||
| AttributeError, | ||
| NotImplementedError, | ||
| pickle.PicklingError, | ||
| ) as e: | ||
| raise UnicornCacheError( | ||
| f"Cannot cache component '{type(component)}' because it is not picklable: {type(e)}: {e}" | ||
| ) from e | ||
|
|
||
| def __exit__(self, *args): | ||
| for component, request, extra_context in self._state.values(): | ||
| component.request = request | ||
| if extra_context: | ||
| component.extra_context = extra_context | ||
|
|
||
|
|
||
| def get_type_hints(obj) -> Dict: | ||
|
|
||
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.
I'm relatively new to Django (in the last year) so I'm not too sure on how bad this would be to add to the context.
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.
I think this might be fine, but I do wonder if it exposes some data that shouldn't be available in the template. What data did you need in the template? If it was just
parentandchildren, we could explicitly add them to the context? The other approach is addcomponentlike you have, but deprecatecomponent_id,component_key,component_namesince that would be duplicative going forward.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.
I thought of deprecating those three component pieces, but figured it might break anyone dependent on them. If you're going to bump the minor version, it seems like a safer change to deprecate them.
On what I need component for: I'm using it to set the 'implied' parent. Basically I found that view was always only set to the highest level component, so it wouldn't work to pass in
parent=viewpast the first level. Since this unicorn info is set in the context as it goes down the tree, it was the easiest way to get the parent to each child at each level. I could have used component_id/key/name to look up the parent again, but that seems inefficient when the parent was just called before the child.It does expose the whole component to the template when all I really need it for is to set the parent of each child. However I didn't think of a cleaner way that wouldn't add look ups or complications.