Results can be sorted in an ascending order or descending order by any key present in the results.
Details
- Reviewers
jskladan - Group Reviewers
resultsdb - Commits
- rRSDB6ccbbed6c109: Sorting by key plugin for browse results collection
Added a couple of testcases.
Diff Detail
- Repository
- rRSDB resultsdb
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Some minor issues, mostly WRT keeping the new code in the same style as the already existing pieces. My only actual functional issue here is that I'd probably just plain allowed _sort to manipulate submit_time - other fields of the Result object are IMO not really that relevant for custom-sorting, and the "exciting" user data (which I suppose could be more interesting to sort by) is stored in ResultData (and this inaccessible by this patch) anyway.
The idea behind the code is pretty sound, and I love that you also added unit tests! I just believe that from the implementation/usability standpoint, it is better to just say "you can sort by the time asc/desc" to keep the expectations right. If it makes any sense.
resultsdb/controllers/api_v2.py | ||
---|---|---|
315–327 | Rather than having the default value of _sort='desc:submit_time', make it None, and change the underlying code respectively (None equals default behaviour). | |
320 | Instead of non-capturing group, you could just r'^(?P<order>asc|desc):(?P<column>.+)$' or even r'^(?P<order>asc|desc):(?P<column>%s)$' % ('|'.join(db_columns)) and just have the regexp sort the "is this one of the columns I deem allowed" for you. Ideally, I'd just honestly allow sorting only by submit_time. Given how the data is structured, it is the (IMO) only field directly from the Result object, which makes sense to sort-by. And if I wanted to sort by the extra data (ResultData) then the whole code would need to be rehauled anyway. | |
323 | While this is very clever, I'd rather see a bit more explicit sort_order = {'asc': db.asc, 'desc': db.desc}[sort_match.group('order')]. | |
435 | As in the comment above. In the spirit of keeping the code style, please have the default value be an empty string, and just ensure the default behavior in that case. | |
478 | ditto |
Instead of non-capturing group, you could just r'^(?P<order>asc|desc):(?P<column>.+)$' or even r'^(?P<order>asc|desc):(?P<column>%s)$' % ('|'.join(db_columns)) and just have the regexp sort the "is this one of the columns I deem allowed" for you.
Ideally, I'd just honestly allow sorting only by submit_time. Given how the data is structured, it is the (IMO) only field directly from the Result object, which makes sense to sort-by. And if I wanted to sort by the extra data (ResultData) then the whole code would need to be rehauled anyway.