-
Notifications
You must be signed in to change notification settings - Fork 14
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
Feature/add optional delayed tasks #45
Conversation
if execute_unless: | ||
assert callable( | ||
execute_unless | ||
), "execute_unless MUST be an instance method that takes no arguments" |
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.
Should this be a little more strict if it /must/ be an instance method? Something like ensuring self == execute_unless.__self__
?
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.
added an additional check using inspect
- we can't check for __self__
because the method isn't bound in this context
@@ -392,6 +400,9 @@ def _resolve_maybe_callable(self, owner_instance, maybe_callable): | |||
return maybe_callable | |||
|
|||
def _run_task(self, owner_instance): | |||
if self.execute_unless and self.execute_unless(owner_instance): |
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.
This could still result in a type error (execute_unless
has been given a non-callable value after the fact). Think that case should be handled?
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.
We would handle it by catching the error and raising a new error, which doesn't seem fundamentally different from just letting the original error propagate. The developer would have to be up to some pretty serious shenanigans in order to trigger this case; I don't mind punishing them with the underlying TypeError in this case :-)
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.
Fair enough.
80493bf
to
693d66e
Compare
693d66e
to
9be3315
Compare
CHANGELOG.md
Outdated
@@ -5,6 +5,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
and this project adheres to [Semantic | |||
Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|||
# [Unreleased] |
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.
Shouldn't this be updated as well?
ce05c6c
to
d47998d
Compare
Adds an optional
execute_unless
argument todeferrable_task
decorator.execute_unless
accepts an EndpointDefinition class method that takes no parameters (exceptself
). Ifexecute_unless
returnsTrue
, the deferrable task will be skipped entirely, rather than scheduled.This is useful in cases where the endpoint may sometimes not need to execute the deferred task at all, depending on the parameters of the request. Implementing
execute_unless
allows the server to avoid the overhead of scheduling and executing a deferred task that is unnecessary.