feat: add stop param to method save #43
Conversation
hugobrilhante
force-pushed
the
feat/add-stop-param-to-save
branch
3 times, most recently
from
2440793 to
7f31c1b
Compare
hugobrilhante
force-pushed
the
feat/add-stop-param-to-save
branch
from
7f31c1b to
0089f4c
Compare
| @@ -125,7 +125,6 @@ def my_serializer_1(obj): | |||
| self.assertEqual("", published[1].body["last_name"]) | |||
| self.assertEqual("", published[1].body["first_name"]) | |||
| self.assertIsNone(published[1].body["last_login"]) | |||
| self.assertAlmostEqual(date_joined.strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3], published[1].body["date_joined"]) | |||
There was a problem hiding this comment.
nitpick: what do you think about use a freezegun ever than remove the line ?
There was a problem hiding this comment.
In my view freezegun is a fantastic lib and it would really be a good idea to use it. But I think that in this case it would not be worth it because it is a single line of information that is not so critical. But like I said, it's my vision. Would you like to add another lib as a dependency for this test?
| @@ -1,3 +1,4 @@ | |||
| # pylint: disable=protected-access | |||
There was a problem hiding this comment.
question: What is the motivation to include this bypass? Not is possible to remove that?
There was a problem hiding this comment.
In this case I could move it to the lines where there is access, I cannot remove it because I need to access the _meta.
| return decorator_publish | ||
| def wrapper(cls): | ||
| class PublishModel(cls): | ||
| def save(self, *args, stop=False, **kwargs): |
There was a problem hiding this comment.
suggestion: Change the stop to more clarify parameter, for example not_publish_message_to_queue. What do you think about this?
One docstring here joined with the documentation can be useful.
There was a problem hiding this comment.
Amazing sugestion
There was a problem hiding this comment.
even though this suggestion would bring more readability, being sucint while naming function arguments sounds a good practice. if you really like to rename this arg, maybe you could call it send_to_queue, with default True value.
This would not break the reading of the method with a negative condition. What do you guys think on this?
| return decorator_publish | ||
| def wrapper(cls): | ||
| class PublishModel(cls): | ||
| def save(self, *args, stop=False, **kwargs): |
There was a problem hiding this comment.
even though this suggestion would bring more readability, being sucint while naming function arguments sounds a good practice. if you really like to rename this arg, maybe you could call it send_to_queue, with default True value.
This would not break the reading of the method with a negative condition. What do you guys think on this?
| @@ -188,6 +188,24 @@ This generates the following data to be sent. | |||
| Published(destination='/topic/my_route_key_1', body='{"id": 1, "one": "Field One"}') | |||
| Published(destination='/topic/my_route_key_2', body='{"id": 1, "two": "Field Two"}') | |||
| ``` | |||
| ## Stopping Publication | |||
|
|
|||
| The `@publish` decorator adds a named parameter called `stop` to the `save` method, which is useful when you need to update an instance of a decorated model and do not want this update to be published on the broker. | |||
There was a problem hiding this comment.
Nice catch!
I would only suggest to rename stop to send_to_queue with default value True. This would bring a little more clearance to the intent of the argument.
Sometimes you need to update an instance of a model decorated with @publish, but you don't want a new message to be published on the broker.
An example, which may serve as a guideline to understand the motivation behind this new feature, would be updating the status of an order when it reaches its final stage:
In this example, by using
stop=Truewhen calling thesavemethod, the message publication on the broker is halted, allowing the update of the order status without triggering a new publication message.This pull request also incorporates the changes from the fix in pull request PR #42