-
Notifications
You must be signed in to change notification settings - Fork 78
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
DJANGO_DRAMATIQ_TASKS_NOT_WRITES, DJANGO_DRAMATIQ_TASKS_WRITES_ONLY #163
base: master
Are you sure you want to change the base?
Conversation
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.
Hey @ikvk ! Thanks for another contribution! Would you mind opening an issue related to this so we can properly track the feature?
Thanks so much!
|
||
class TaskManager(models.Manager): | ||
def create_or_update_from_message(self, message, **extra_fields): | ||
def create_or_update_from_message(self, message, **extra_fields) -> Optional['Task']: |
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.
nitpick:
def create_or_update_from_message(self, message, **extra_fields) -> Optional['Task']: | |
def create_or_update_from_message(self, message, **extra_fields): |
I dont believe we are using types in this project. Can you please remove this for consistency? 😄
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.
I suggest adding types everywhere, it always makes it easier to read the code. Compatibility will not be affected: in the library "Requirements Python 3.9+"
def create_or_update_from_message(self, message, **extra_fields) -> Optional['Task']: | ||
|
||
# black and write lists | ||
if DJANGO_DRAMATIQ_TASKS_WRITES_ONLY and message.actor_name not in DJANGO_DRAMATIQ_TASKS_WRITES_ONLY: |
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.
suggestion: Since you default to an empty list in the getattr method above, the first part of this conditional should not be necessary.
if DJANGO_DRAMATIQ_TASKS_WRITES_ONLY and message.actor_name not in DJANGO_DRAMATIQ_TASKS_WRITES_ONLY: | |
if message.actor_name not in DJANGO_DRAMATIQ_TASKS_WRITES_ONLY: |
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.
False-like vals will not works this way. For example - None
def create_or_update_from_message(self, message, **extra_fields): | ||
def create_or_update_from_message(self, message, **extra_fields) -> Optional['Task']: | ||
|
||
# black and write lists |
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.
nitpick:
# black and write lists |
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.
okay
Done |
Allow to specify which actors to write to the database with "black" and "white" lists: