-
Notifications
You must be signed in to change notification settings - Fork 36
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
ENH: Respect sort and limit arguments #158
Conversation
4edf97b
to
caba703
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable
You'll also need to update the docblocks to include the new params
Updated dockblocks and fixed the |
Fixed the test. The old code was calling sort but not actually using the resultant sorted list - so the test was asserting that an incorrect order was correct. |
We should keep the default sort of ParentID so that it matches how it previously behaved |
In that case we should remove that sort, because the previous behaviour wasn't actually sorting. Sort was called on the list but that returns a sorted list - it doesn't sort the list in place. And the sorted list wasn't used in the old code. So the actual list used for the report wasn't sorted. |
Oh right, yes good point |
These parameters are defined in the PHPDocs for `Report` and are technically part of the method signature. They should be respected and in the case of the new default limit in silverstripe/silverstripe-reports#139 this could have performance ramifications for large datasets.
Removed the default sort, as it was a departure from the existing behaviour. |
These parameters are defined in the PHPDocs for
Report
and are technically part of the method signature. They should be respected and in the case of the new default limit in silverstripe/silverstripe-reports#139 this could have performance ramifications for large datasets.Note that
filterByCallback
applies the limit from theDataList