feat: [sc-207225] [SAP-OData] Implement server side pagination #13
Conversation
| @@ -21,10 +21,11 @@ def __init__(self, config, plugin_config): | |||
| object 'plugin_config' to the constructor | |||
| """ | |||
| Connector.__init__(self, config, plugin_config) | |||
| logger.info("Starting SAP-OData v1.0.3") | |||
| logger.info("Starting SAP-OData v1.0.4-beta.3") | |||
There was a problem hiding this comment.
changed in c798cc8
| if self.is_client_side_pagination(): | ||
| if self.bulk_size == 0: | ||
| return None | ||
| bulk_size = self.bulk_size | ||
| if records_limit > 0: | ||
| bulk_size = records_limit if records_limit < bulk_size else bulk_size | ||
| else: | ||
| bulk_size = None | ||
| return bulk_size |
There was a problem hiding this comment.
Optional:
| if self.is_client_side_pagination(): | |
| if self.bulk_size == 0: | |
| return None | |
| bulk_size = self.bulk_size | |
| if records_limit > 0: | |
| bulk_size = records_limit if records_limit < bulk_size else bulk_size | |
| else: | |
| bulk_size = None | |
| return bulk_size | |
| if not self.is_client_side_pagination() or self.bulk_size <= 0: | |
| return None | |
| # Unlimited records_limit. Default to bulk_size | |
| if records_limit < 0: | |
| return self.bulk_size | |
| return min(self.bulk_size, records_limit) |
| if skip is None: | ||
| skip = 0 |
There was a problem hiding this comment.
:question Why not initialize skip to 0?
There was a problem hiding this comment.
if skip==0, the ?skip=0 query string is sent to the request, which is something we want to avoid if no client side pagination is being used.
|
|
||
| class RecordsLimit(): | ||
| def __init__(self, records_limit=-1): | ||
| self.has_no_limit = (records_limit == -1) |
There was a problem hiding this comment.
Since this property is only used in is_reached, I think it's better to isolate it in the same condition in case the property is modified and has_no_limit is not updated
python-lib/odata_common.py
Outdated
| self.records_limit = records_limit | ||
| self.counter = 0 | ||
|
|
||
| def is_reached(self): |
There was a problem hiding this comment.
Naming is kinda confusing, I did not expect it to increment the counter. Imo you can call separate the increment into another method
There was a problem hiding this comment.
fixed in c798cc8
Story details: https://app.shortcut.com/dataiku/story/207225