#213 paginated products #238
Conversation
catalog/newcontext/tags.py
Outdated
| @@ -17,9 +17,9 @@ def context(self): | |||
|
|
|||
| class ParsedTags(Tags): | |||
|
|
|||
| def __init__(self, tags: Tags, req_kwargs): | |||
| def __init__(self, tags: Tags, raw_tags: str=''): | |||
There was a problem hiding this comment.
with default value '' we can miss type annotation. That's what we are usually doing now.
But we has no convention for it, it's up to you
tests/catalog/test_context.py
Outdated
|
|
||
| def test_paginated_qs(self): | ||
| with override_settings(CATEGORY_STEP_MULTIPLIERS=[30]): | ||
| products = self.products_ctx() |
There was a problem hiding this comment.
we can call products_ctx as just the context. We are already in ProductsContext class
There was a problem hiding this comment.
It will raise name conflicts with module context, so i'd prefer to leave it as it is. Should i create a todo for it?
There was a problem hiding this comment.
@artemiy312 , it'll not lead to conflict, because it's method but not a function.
Create todo plz, y
tests/catalog/test_context.py
Outdated
| ) | ||
|
|
||
| def test_paginated_qs(self): | ||
| with override_settings(CATEGORY_STEP_MULTIPLIERS=[30]): |
There was a problem hiding this comment.
we have const 30 below as per_page var.
Define the var before with block or move value 30 to class level const
| def qs(self): | ||
| return self._pagination_context()['page'].object_list | ||
|
|
||
| def context(self): |
There was a problem hiding this comment.
annotate it's returning value with dict plz
There was a problem hiding this comment.
It is implementation of base class' abstract method, that already have the annotation and it won't be changed. Should we duplicate this information?
There was a problem hiding this comment.
@artemiy312 , i think we should. Because when we read code, we don't walk throw ancestors of class we are reading
| def _pagination_context(self): | ||
| return self._pagination.context() | ||
|
|
||
| def qs(self): |
There was a problem hiding this comment.
annotate it's with QuerySet plz
|
|
||
| def context(self): | ||
| return { | ||
| **self._products.context(), |
There was a problem hiding this comment.
maybe we should remove this string?
It seems to me, that every context class ancestor should return only it's part of context.
But maybe i don't fully understand how context instances should chain in a view finally.
So, it's up to you
There was a problem hiding this comment.
It just helps to avoid key conflicts, that would be occurred. For example we already have a copy of page instance in final context, but pagination has page too
There was a problem hiding this comment.
@artemiy312 , so similar keys should be merged. Python dict already has such strategy and it looks clear:
In [1]: {**{1: 'one', 2: 'two'}, **{1: 'uno'}}
Out[1]: {1: 'uno', 2: 'two'}
catalog/newcontext/products.py
Outdated
| raise http.Http404('Page does not exist.') | ||
|
|
||
| self._products = products | ||
| self._url = url |
There was a problem hiding this comment.
seems we don't use url at this class
tests/catalog/test_context.py
Outdated
| context.products.PaginatedProducts(None, '', page_number, per_page-1) | ||
|
|
||
| with self.assertRaises(Http404): | ||
| # page_number < 1 |
There was a problem hiding this comment.
not existing page number might be more clear
tests/catalog/test_context.py
Outdated
| @@ -55,13 +83,13 @@ class TagsContext(TestCase): | |||
|
|
|||
| def test_parsed_tags(self): | |||
| tags_ctx = mocked_ctx() | |||
| context.tags.ParsedTags(tags_ctx, {'tags': 'test'}).qs() | |||
| context.tags.ParsedTags(tags_ctx, 'test').qs() | |||
There was a problem hiding this comment.
let's use named arg for 'test' it might be more clear
Closes #213