[FEATURE] Add sort_ascending to BatchDefinition fluent API #9756
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #9756 +/- ##
===========================================
- Coverage 82.68% 82.68% -0.01%
===========================================
Files 512 512
Lines 46896 46896
===========================================
- Hits 38778 38777 -1
- Misses 8118 8119 +1 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| batch_definition = asset.add_batch_definition_daily( | ||
| name=name, column=column, sort_ascending=sort_ascending | ||
| ) |
There was a problem hiding this comment.
The output of this test doesn't change based on the new param right? Could we add a test where the order matters?
There was a problem hiding this comment.
thanks @cdkini! These methods just return a BatchDefinition, not a list of batches; that functionality is tested in test_get_batch_list_from_batch_request__sort_ascending/descending. I created a followup ticket to add integration tests around the fluent batch def api.
There was a problem hiding this comment.
I will add a comment in that ticket that explicitly lists testing sorters as part of the AC.
Recent changes added a
sort_ascending: boolparameter to Partitioners, so this PR allows defining that from the BatchDefinition fluent API.