Add easy way to annotate Route QS with "level" #131

meshy · 2018-01-18T23:18:45Z

In order to create a navigation on a test project, I have had to add the following code:

# context_processors.py

from django.db.models.expressions import Func
from pages.models import Page


class CharCount(Func):
    template = "CHAR_LENGTH(%(field)s) - CHAR_LENGTH(REPLACE(%(field)s, '%(char)s', ''))"

    def __init__(self, field, *, char, **extra):
        if len(char) != 1:
            raise ValueError('CharCount can only count individual chars.')
        extra['char'] = char
        super().__init__(field, **extra)


def navigation_pages(request):
    nav_pages = (
        Page.objects
        .annotate(level=CharCount('url', char='/'))
        .filter(level=2)
    )
    return {'navigation_pages': nav_pages}

I think it would be nice to have this level-annotation functionality built into RouteManager as an optional addition. Something like Route.objects.with_levels().filter(level=2) strikes me as nicer.

Update. Okay, I've made this a PR now. It doesn't have exactly the same interface as above. See comment below for new syntax.

meshy · 2018-01-18T23:20:48Z

This allows for Route.objects.with_level(2), or Route.objects.with_level() if no filtering is required.

meshy · 2018-01-18T23:30:58Z

I might as well add a @property to make the level always accessible...

meshy · 2018-01-19T23:20:48Z

conman/routes/managers.py

+        """
+        Annotate the queryset with the level of each item.
+
+        The level reflects the number of forward slashes in the path.


The level is one-indexed; it reflects the number of forward slashes in the path.

LilyFoote · 2018-01-20T14:28:39Z

tests/routes/test_expressions.py

+        expected = (
+            'SELECT "routes_route"."id", ' +
+            'CHAR_LENGTH("routes_route"."url") - ' +
+            'CHAR_LENGTH(REPLACE("routes_route"."url", \'/\', \'\')) AS "level" ' +


Perhaps use a triple quoted string to avoid the need to escape any quotes.

Good plan, thanks.

LilyFoote · 2018-01-20T14:31:10Z

tests/routes/test_expressions.py

+            CharCount('url', char='no')
+
+        with self.assertRaisesMessage(ValueError, msg):
+            CharCount('url', char='')


Perhaps this is a place to use subTest.

LilyFoote · 2018-01-20T14:32:08Z

tests/routes/test_expressions.py

+    def test_calling_format(self):
+        """Ensure the 'char' argument is always a keyword-arg."""
+        with self.assertRaises(TypeError):
+            CharCount('url', 'unacceptable')


It might be cleaner to use a single character here. so we're violating only one assumption of the api.

LilyFoote · 2018-01-20T14:33:28Z

tests/routes/test_managers.py

+        result = Route.objects.with_level().order_by('level')
+        self.assertEqual(result[0].level, 1)
+        self.assertEqual(result[1].level, 2)
+        self.assertEqual(result[2].level, 3)


Perhaps:

self.assertSequenceEqual(result.values_list('level', flat=True), [1, 2, 3])

LilyFoote · 2018-01-20T14:36:53Z

tests/routes/test_expressions.py

+            'CHAR_LENGTH(REPLACE("routes_route"."url", \'/\', \'\')) AS "level" ' +
+            'FROM "routes_route"'
+        )
+        self.assertEqual(str(qs.query), expected)


str(qs.query) isn't perfect, since it does the interpolation of any params in python, not in sql. It might be worth using connection.queries instead.

Thanks for the heads up -- I'll give that a shot.

meshy · 2018-01-21T18:20:11Z

@Ian-Foote thanks for the review. I've rebased with your (slightly altered) changes, and added a Route.level property too.

How does that look?

LilyFoote · 2018-01-21T19:10:11Z

conman/routes/models.py

+    @level.setter
+    def level(self, new_value):
+        """Silently fails to allow queryset annotation to work."""
+        pass


Are you sure the annotation is used instead of the property if it exists? Or does that not matter?

It's not that the annotation is used, exactly, it's that when an annotated object is instantiated, the attribute is set. Without this, and with the @property, a "can't set attribute" error is raised.

I see - the annotation is used for sql-level operations like filtering, but the descriptor whenever we're in python.

LilyFoote · 2018-01-21T19:16:24Z

conman/routes/managers.py

+
+    def with_level(self, level=None):
+        """
+        Annotate the queryset with the (1-indexed) level of each item.


I see why you chose 1-indexed, but I'm not sure that's the best api. It seems more natural to me to care about how many parts the url has rather than the number of / separators.

Yeah, I wasn't sure about it myself. I went for this simply because the implementation was marginally easier -- you'll probably not be the last person to think it's strange -- I'll look into changing it.

LilyFoote · 2018-01-21T19:18:26Z

conman/routes/managers.py

+        """
+        Annotate the queryset with the (1-indexed) level of each item.
+
+        The level reflects the number of forward slashes in the path.


Do we guarantee elsewhere that a url always starts and ends with a /?

Yes, it's a strange necessity of conman -- it makes other similar operations easy.

On the whole, it shouldn't cause too many issues, I hope.

meshy · 2018-01-21T22:00:08Z

Thanks :)

meshy added the New functionality label Jan 5, 2018

meshy mentioned this pull request Jan 7, 2018

Add Route.get_children #123

Open

meshy force-pushed the level-annotation branch from df82e44 to 31745dc Compare January 18, 2018 23:22

meshy commented Jan 19, 2018

View reviewed changes

LilyFoote approved these changes Jan 20, 2018

View reviewed changes

meshy force-pushed the level-annotation branch from 31745dc to 319d6c8 Compare January 21, 2018 18:19

meshy added 2 commits January 21, 2018 18:22

Add shortcut to annotate and filter Routes by level

3d16700

Add Route.level property

9a9c0da

meshy force-pushed the level-annotation branch from 319d6c8 to 9a9c0da Compare January 21, 2018 18:23

LilyFoote reviewed Jan 21, 2018

View reviewed changes

Change Route.level to be zero-indexed

d3963e4

LilyFoote approved these changes Jan 21, 2018

View reviewed changes

meshy merged commit 516f26a into master Jan 21, 2018

meshy deleted the level-annotation branch January 21, 2018 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add easy way to annotate Route QS with "level" #131

Add easy way to annotate Route QS with "level" #131

meshy commented Jan 18, 2018 •

edited

Loading

meshy commented Jan 18, 2018 •

edited

Loading

meshy commented Jan 18, 2018

meshy Jan 19, 2018

LilyFoote Jan 20, 2018

meshy Jan 20, 2018

LilyFoote Jan 20, 2018

LilyFoote Jan 20, 2018

LilyFoote Jan 20, 2018

LilyFoote Jan 20, 2018

meshy Jan 20, 2018

meshy commented Jan 21, 2018

LilyFoote Jan 21, 2018

meshy Jan 21, 2018

LilyFoote Jan 21, 2018

meshy Jan 21, 2018

LilyFoote Jan 21, 2018

meshy Jan 21, 2018

meshy Jan 21, 2018

LilyFoote Jan 21, 2018

meshy Jan 21, 2018

meshy Jan 21, 2018

meshy commented Jan 21, 2018

Add easy way to annotate Route QS with "level" #131

Add easy way to annotate Route QS with "level" #131

Conversation

meshy commented Jan 18, 2018 • edited Loading

meshy commented Jan 18, 2018 • edited Loading

meshy commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshy commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshy commented Jan 21, 2018

meshy commented Jan 18, 2018 •

edited

Loading

meshy commented Jan 18, 2018 •

edited

Loading