For backward compatibility, relax the requirements that transaction meta data (user or description) be text #44
Conversation
meta data (user or description) be text
transaction/_transaction.py
Outdated
| @@ -22,6 +23,7 @@ | |||
| from transaction.weakset import WeakSet | |||
| from transaction.interfaces import TransactionFailedError | |||
| from transaction import interfaces | |||
| from transaction._compat import binary_type | |||
There was a problem hiding this comment.
You can replace binary_type with bytes everywhere. On Python2:
>>> bytes is str
True
There was a problem hiding this comment.
Sure. I was just being consistent with the rest of the package.
I'll change the rest of the package to use bytes.
There was a problem hiding this comment.
ha, except it's not used by anything else, so it wasn't used before this pr.
transaction/_transaction.py
Outdated
| if isinstance(s, bytes): | ||
| return s.decode('utf-8', 'replace') | ||
| else: | ||
| return text_type(s) |
There was a problem hiding this comment.
So everything besides text and str is ignored – was this intended?
There was a problem hiding this comment.
No, nothing is ignored here. (None is ignored at the call sites.)
Anything but text generates a warning. Bytes get special treatment by decoding them. Everything else gets "stred" by converting them to text. The later because on the transaction mailing list, somone asserted that Plone sometimes passes ints. .
There was a problem hiding this comment.
FWIW, RelStorage 1.6 simply ran str(txn.user) and str(txn.description), so it could take anything for those values.
There was a problem hiding this comment.
On Python 3:
>>> str(b'test')
"b'test'"
which seems suboptimal.
There was a problem hiding this comment.
RelStorage didn't check for None before it ran str, so if user and description were ever set to that (which seems likely, especially for user), there would be 'None' in the database.
My point is, it probably doesn't matter that much 😄
transaction/_transaction.py
Outdated
| if isinstance(s, text_type): | ||
| return s | ||
|
|
||
| warnings.warn("Expected text", DeprecationWarning, stacklevel=3) |
There was a problem hiding this comment.
It would be nice here to see the actual value which caused the warning.
There was a problem hiding this comment.
Unfortunately, this will cause memory leaks in "warning-once" mode, because the text is part of the key to remember whether a warning was issued or not. This applies to both the builtin (_warnings) and python (warnings) versions.
There was a problem hiding this comment.
OK, I undid this in 2.1.2.
|
+1, looks good. |
|
Releases as 2.1.1. |
No description provided.