Add API for receiving asynchronous notices #147
Conversation
It is identical to the regular severity "S", but it is never localized. Introduced in Postgres-9.6: https://www.postgresql.org/docs/9.6/static/protocol-error-fields.html
| We already have |
| I've just replaced the function to |
| # Ignore new unknown fields | ||
| pass | ||
| | ||
| self.connection._notice(message) |
There was a problem hiding this comment.
A subclass of PostgresMessage should be returned here instead of a plain dict.
There was a problem hiding this comment.
I thought that way, but why don't pass just a dict?
It is easier for logging (people don't need to iterate over an instance of a class), and callbacks don't get anything except a message consists of fields and values.
PostgresMessage is designed for exceptions, where it is reasonable to have all known fields as attributes, notices are lightweight, used at specified places (callbacks) and don't need so complex creation process (and reverse process -- iteration via attributes to create a dict to be logged).
There was a problem hiding this comment.
A simple class is not much "heavier" than a dict. Furthermore, notices should be a rare occurrence (compared to Notify). I don't really think that the ability to enumerate fields is all that useful. On the other hand, if notices are instances of PostgresNotice object, we can make its __str__ default to message and provide access to the message dict via the as_dict() method. IMO, it's a much nicer interface than a plain dict.
asyncpg/connection.py Outdated
| await self.fetch('UNLISTEN {}'.format(channel)) | ||
| | ||
| def add_notice_callback(self, callback): | ||
| """Set a callback to be used when asyncronous NoticeResponse is received |
There was a problem hiding this comment.
The first line of the docstring should be a complete sentence describing what the method does.
Then an empty new line.
Then you can add more paragraphs.
asyncpg/connection.py Outdated
| self._loop.create_task(cancel()) | ||
| | ||
| def _notice(self, message): | ||
| # `self._notice_cb` is checked internally and passed here as the `cb` |
There was a problem hiding this comment.
Add a 'period' at the end of comment sentences. Also the comment looks outdated.
asyncpg/connection.py Outdated
| self._listeners = {} | ||
| self._aborted = True | ||
| await self._protocol.close() | ||
| self._notice_cb_set = set() |
There was a problem hiding this comment.
I'd rename it to _notice_callbacks.
| | ||
| self._loop.create_task(cancel()) | ||
| | ||
| def _notice(self, message): |
There was a problem hiding this comment.
I'd rename this to _schedule_notice_callbacks.
There was a problem hiding this comment.
Disagree. They are called immediately, there is no schedule process.
There was a problem hiding this comment.
Right. Well, you should use loop.call_soon anyways. We never want to call callback right away.
| # Ignore new unknown fields | ||
| pass | ||
| | ||
| self.connection._notice(message) |
asyncpg/connection.py Outdated
| | ||
| for cb in self._notice_cb_set: | ||
| try: | ||
| cb(con_ref, message) |
There was a problem hiding this comment.
And we'll update notify to use call_soon in a separate PR. That's something that we just oversaw.
Sometimes it is useful to get all non-None members, usually for logging. Do it in a base class to avoid copy-pasting in users' code.
Notice message types are: WARNING, NOTICE, DEBUG, INFO, or LOG. https://www.postgresql.org/docs/current/static/protocol-error-fields.html Issue MagicStack#144.
| Hi @vitaly-burovoy! Are you going to continue working on this PR? We'll be releasing a new version of asyncpg soon. |
vitaly-burovoy force-pushed the notices branch 2 times, most recently from 6934885 to 844804a Compare | I committed requested changes (I hope I did not forget something), but Travis could not check it because it failed installing Python in OSX (if I understand its log correctly). What should I do now? |
| I'm merging the PR although there's some work left; we'll open separate PRs to address it. |
| Thank you very much! |
3 participants
Per request #144.
It seems I implemented it long time ago, but did not have enough time to finalize it as a PR.
I think a single callback per connection is enough, therefore API has just
set_notice_callbackinstead ofadd_notice_callbackandremove_notice_callback.P.S.: Here is also two minor commits small enough to create separated PRs.