make warnings/logging capture threadsafe #108
Conversation
matthiasdiener
force-pushed
the
warn-log-threadsafe
branch
from
ed26071 to
6336b0e
Compare
|
|
||
| def save_warnings(self) -> None: | ||
| for w in self.warning_data: | ||
| self.db_conn.execute( |
There was a problem hiding this comment.
It seems support for old schema versions was lost here.
There was a problem hiding this comment.
I can restore that support if you prefer, but is it useful to be able to write older schema versions? _set_up_schema is only able to create the newest schema version at the moment.
There was a problem hiding this comment.
I'm not opposed to entirely dropping that support even, but there have to be error messages that clarify what happened, not failures/incorrect execution of INSERT somewhere in the guts.
There was a problem hiding this comment.
Ok, I think I understand. In other words, warnings/logging capture should only be enabled for the latest schema version; if for some reason an older schema version is used, the capture should be disabled with a warning message?
There was a problem hiding this comment.
Or you can just error outright on open.
There was a problem hiding this comment.
What do you think of 30ed3fb?
There was a problem hiding this comment.
What I meant was to drop support for older schemas entirely, with an appropriate error message. Note that this should probably be a separate PR. At that point, all conditionals based on schema_version should probably go away.
There was a problem hiding this comment.
Ok, I added this as issue #109.
logpyle/__init__.py
Outdated
| if self.schema_version < 3: | ||
| raise ValueError("Logging capture needs at least schema_version 3, " | ||
| f" got {self.schema_version}") |
There was a problem hiding this comment.
Shouldn't these be in capture_xxx?
There was a problem hiding this comment.
I moved them in cd8301f
Fixes #106. Alternative to #107.