Summary

LocalDataQuerySet order_by implementation

Review Request #7200 — Created April 14, 2015 and submitted April 16, 2015, 4:54 p.m.

Information

Owner

VTL-Developer

Repository

Djblets

Branch

master

Bugs

Depends On

Commit

c895f39...

Reviewers

Groups

djblets

People

Description

Queries or calls may want items in a LocalDataQuerySet to be ordered for usage. To help bridge the gap between LocalDataQuerySet and a Django queryset, an order_by function has been created to order items in the LocalDataQuerySet by the specified attributes, similar to Django queryset.

Testing Done

Test case for order_by.

Issues

Description	From	Last Updated
This should probably be a member of the LocalDataQuerySet class. The _COMPILED bit is unnecessary.	brennie	April 14, 2015, 5:19 p.m.
Can we use ascending and descending here (instead of non-descending and non-ascending) ?	brennie	April 14, 2015, 5:19 p.m.
This doesn't need a leading underscore. It won't be visible outside this function.	brennie	April 14, 2015, 5:19 p.m.
Can this be written to use iteration instead of recursion?	brennie	April 14, 2015, 5:19 p.m.
I think this should probably clone the data and sort that instead, e.g. copy = self.clone() copy._data.sort(...) return copy	brennie	April 14, 2015, 5:19 p.m.
Col: 57 E231 missing whitespace after ','	reviewbot	April 14, 2015, 4:47 p.m.
Col: 59 E231 missing whitespace after ','	reviewbot	April 14, 2015, 4:47 p.m.
Function names should be verb phrases. How about compare ?	brennie	April 14, 2015, 6:38 p.m.
The first line of a docstring should be written in the imperative mood. How about "Compare two elements of the …	brennie	April 14, 2015, 6:38 p.m.
Since you are using attrs[i] and i, you can change this to use enumerate(attrs).	brennie	April 14, 2015, 6:38 p.m.
Blank line between these.	brennie	April 14, 2015, 6:38 p.m.
Blank line after class docstring.	brennie	April 16, 2015, 1:24 p.m.

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py

djblets/db/tests/test_local_data_query_set.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 57
 E231 missing whitespace after ','
```
djblets/db/tests/test_local_data_query_set.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 59
 E231 missing whitespace after ','
```

Change Summary:

Removed the trailing comma in the arrays for test_order_by.

Commit:

ae7c7254124ea17f423396311a09888da307c84d

22e0de64e9736f0e562830b8a16ba1d4c06489e6

Diff:

Revision 2 (+72)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_local_data_query_set.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py

djblets/db/query.py (Diff revision 1)

The issue has been resolved. Show all issues

This should probably be a member of the LocalDataQuerySet class. The _COMPILED bit is unnecessary.

djblets/db/query.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we use ascending and descending here (instead of non-descending and non-ascending) ?

djblets/db/query.py (Diff revision 1)

The issue has been resolved. Show all issues

This doesn't need a leading underscore. It won't be visible outside this function.

djblets/db/query.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can this be written to use iteration instead of recursion?
```

djblets/db/query.py (Diff revision 1)

The issue has been resolved. Show all issues

I think this should probably clone the data and sort that instead, e.g.

copy = self.clone()
copy._data.sort(...)
return copy

Change Summary:

Changed recursive call to be iterative instead.
Moved global variable as a member of a class instead.
Updated the description to be ascending and descending instead of the "non" versions.
Removed leading underscore for function naming.
Changed xrange to range for Python3 compatability.

Commit:

22e0de64e9736f0e562830b8a16ba1d4c06489e6

918875da7af0c87bbd41d93b2ce353cd9cf735e8

Diff:

Revision 3 (+71)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_local_data_query_set.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py

djblets/db/query.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Function names should be verb phrases. How about compare ?
```

djblets/db/query.py (Diff revision 3)

The issue has been resolved. Show all issues

The first line of a docstring should be written in the imperative mood. How about "Compare two elements of the query set"

djblets/db/query.py (Diff revision 3)

The issue has been resolved. Show all issues

Since you are using attrs[i] and i, you can change this to use enumerate(attrs).

djblets/db/query.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Change Summary:

Updated to documentation and naming of the comparator function.

Used iterator for attributes instead of indexing the attributes.

Commit:

918875da7af0c87bbd41d93b2ce353cd9cf735e8

dc6cb069888c3964b9ea0d77bf33defc845d0aad

Diff:

Revision 4 (+72)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_local_data_query_set.py

Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py

Ship it!

```
Ship It!
```

brennie April 14, 2015, 6:57 p.m.

Sorry, I missed one thing. After this is fixed, I'll land it.

djblets/db/query.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line after class docstring.
```

Change Summary:

Added blank line and removed the start (default is already 0) argument for range in the test cases.

Commit:

dc6cb069888c3964b9ea0d77bf33defc845d0aad

c895f39c37e3de8b4d5292fc5ef189769f7cad38

Diff:

Revision 5 (+73)

Show changes

	djblets/db/query.py
	djblets/db/tests/test_local_data_query_set.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py



Tool: Pyflakes
Processed Files:
    djblets/db/query.py
    djblets/db/tests/test_local_data_query_set.py

Status:: Completed
Change Summary:: Pushed to master (ba64942)