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

Allow group_by arguments in search requests #353

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@sauloal
Contributor

sauloal commented Aug 22, 2014

order_by parameter

order_by
order_by parameter

@jfinkels jfinkels changed the title from order_by to Allow group_by arguments in search requests Aug 22, 2014

@jfinkels

This comment has been minimized.

Owner

jfinkels commented Aug 22, 2014

I think you mean group_by instead of order_by (the latter already exists).

@@ -127,6 +127,21 @@ def __repr__(self):
return '<OrderBy {0}, {1}>'.format(self.field, self.direction)
class GroupBy(object):
"""Represents an "order by" in a SQL query expression."""

This comment has been minimized.

@jfinkels

jfinkels Aug 22, 2014

Owner

Change an "order by" to a "group by".

def __init__(self, field):
"""Instantiates this object with the specified attributes.
`field` is the name of the field by which to order the result set.

This comment has been minimized.

@jfinkels

jfinkels Aug 22, 2014

Owner

Change "order" to "group".

@@ -219,6 +234,10 @@ def __init__(self, filters=None, limit=None, offset=None, order_by=None,
ordering directives to apply to the result set which matches the
search.
`group_by` is a list of :class:`GroupBy` objects, representing the
groupin directives to apply to the result set which matches the

This comment has been minimized.

@jfinkels

jfinkels Aug 22, 2014

Owner

Change "groupin" to "grouping".

Change "which" to "that".

@@ -257,6 +279,8 @@ def from_dictionary(dictionary):
(in dictionary form), ``dictionary['order_by']`` is the list of
:class:`OrderBy` objects (in dictionary form), ``dictionary['limit']``
is the maximum number of matching entries to return,
``dictionary['group_by']`` is the list of

This comment has been minimized.

@jfinkels

jfinkels Aug 22, 2014

Owner

It looks like line breaks here are too aggressive.

@@ -429,6 +456,12 @@ def create_query(session, model, search_params):
query = query.limit(search_params.limit)
if search_params.offset:
query = query.offset(search_params.offset)

This comment has been minimized.

@jfinkels

jfinkels Aug 22, 2014

Owner

Remove blank lines before and after this if block.

@@ -429,6 +456,12 @@ def create_query(session, model, search_params):
query = query.limit(search_params.limit)
if search_params.offset:
query = query.offset(search_params.offset)
if search_params.group_by:
for val in search_params.group_by:

This comment has been minimized.

@jfinkels

jfinkels Aug 22, 2014

Owner

val is not a good identifier for the GroupBy object. How about groupby?

if search_params.group_by:
for val in search_params.group_by:
field = getattr(model, val.field)

This comment has been minimized.

@jfinkels

jfinkels Aug 22, 2014

Owner

You're going to need to do some error checking here in case model does not have an attribute with the name given in the value of val.field.

@jfinkels

This comment has been minimized.

Owner

jfinkels commented Aug 22, 2014

This is good, but I won't merge this without tests or documentation. Also please attend to the comments I made on the diff.

@jfinkels jfinkels added the enhancement label Feb 3, 2015

@jfinkels

This comment has been minimized.

Owner

jfinkels commented Feb 3, 2015

I fixed the problems with this pull request and merged it in commit 493637b.

@jfinkels jfinkels closed this Feb 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment