feat: generic resource support #125
Conversation
| **kwargs, | ||
| ): | ||
| super().__init__(**kwargs) | ||
| assert ( |
There was a problem hiding this comment.
Should this raise a TypeError?
There was a problem hiding this comment.
My thinking is that because any incorrect usage of this decorator will reveal itself at compile time, was can use an assert rather than raising a TypeError and get the same protection because any issue will be reliably detected in unit tests. Using the assert allows the check to be optimized out when running in production
|
|
||
| retry_params = RetryParams(*retry_params) | ||
| if retry_params.retry_exception_filter and not any( | ||
| [ |
There was a problem hiding this comment.
You could probably drop the square brackets and let it be a generator expression?
|
The test failures are from coverage:
|
| @@ -228,7 +230,9 @@ def get_response(self): # noqa: C901 | |||
|
|
|||
| resource = self.bound_endpoint.resource | |||
|
|
|||
| if hasattr(resource, "is_dirty"): | |||
| if isinstance(resource, django.db.models.Model) and hasattr( | |||
There was a problem hiding this comment.
The idea is that if it's not a Django Model we might not want to update it so often? Updates for those objects will have to be handled outside of DDA?
There was a problem hiding this comment.
yeah, this auto-save functionality is pretty tightly coupled to the resources being Django models. This PR allows using non-django models as resources, but the cost is that the endpoint implementation must handle persistence on its own
| ) | ||
|
|
||
| logger.info( | ||
| "future_task_runner: method=%s, resource_id=%s", |
There was a problem hiding this comment.
Should this be "generic_future_task_runner"?
| if retry_params is None: | ||
| raise | ||
|
|
||
| retry_params = RetryParams(*retry_params) | ||
| if retry_params.retry_exception_filter and not any( | ||
| [ | ||
| f"{e.__class__.__module__}.{e.__class__.__name__}" == ex_type | ||
| for ex_type in retry_params.retry_exception_filter | ||
| ] | ||
| ): | ||
| # this exception is not retryable | ||
| raise | ||
|
|
||
| if retry_params.retries_remaining == 0: | ||
| # out of chances | ||
| raise | ||
|
|
||
| _log_retry_stats(endpoint_method_name, resource_instance_id, correlation_id) |
There was a problem hiding this comment.
This is all identical to the section in future_task_runner right? Could it be extracted as a function? Some below too.
There was a problem hiding this comment.
this part is slightly different because of the different function signatures and how the resource is passed to the task runner. I briefly attempted to extract out some commonality to make it less repetitive, but it didn't yield much. I'll take another look though.
| ) | ||
|
|
||
|
|
||
| def schedule_generic_future_task_runner( |
There was a problem hiding this comment.
This looks identical to schedule_future_task_runner unless I'm missing something.
|
Also, so we don't forget: CHANGELOG addition needed |
pyproject.tomlpip installsucceeds with a clean virtualenv