-
Notifications
You must be signed in to change notification settings - Fork 169
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
Add NatsBroker.new_inbox() #1543
Conversation
Added missing 'and not confluent' to pytest command for running the tests when no broker instances are present.
|
||
[1] https://nats-io.github.io/nats.py/modules.html#nats.aio.client.Client.new_inbox | ||
""" | ||
return self._connection.new_inbox() |
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.
You should add assert here to check, that the broker is already connected (and ignore this asset by noseq)
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.
Thanks for the hint. I only saw this comment after I added a check which raises a RuntimeError. Please let me know if you'd like me to use a regular assert statement instead.
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.
Please, use assert in this case. I prefer to use switchable checks for development.
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.
Gotcha, done. Out of curiosity, what do you mean by using "switchable checks for development"?
Head branch was pushed to by a user without write access
1f77c01
to
0c2447a
Compare
FYI, I added one more line to the docstring (to mention NatsBroker's |
Fixes #1542
Type of change
Please delete options that are not relevant.
Checklist
scripts/lint.sh
shows no errors)scripts/test-cov.sh
scripts/static-analysis.sh
(Note: regarding the last item, running
mypy
produced 24 errors but they are all related to the confluent broker and not to my changes.)