Convert boolean event content to strings for push rule evaluation #12181
Conversation
|
This does seem sensible, but I'm undecided how to handle the spec side of this. As of matrix-org/matrix-spec-proposals#3690 (ie, current unstable spec), the spec contains the words:
Obviously, this PR is in conflict with that. So we've got a couple of ways to proceed:
Strictly speaking I think it should be an MSC, but I'm not going to insist on a bunch of process if folks think that's ridiculous. However, it does bring into question: what should we do about other types of fields? (In particular, numbers and |
The inconsistency between non-string types is what really makes me think the behavior should be specced. It really isn't obvious to me that you would want a boolean (or number) value to be matched as a string also. I'd rather see this go through the MSC process, I think, but would definitely defer to people with more history of how push rules are supposed to work! |
|
While I generally do want to be able to match booleans (and other types), I'm not sure if stringifying them is the right approach. |
| @@ -232,6 +232,8 @@ def _flatten_dict( | |||
| if result is None: | |||
| result = {} | |||
| for key, value in d.items(): | |||
| if isinstance(value, bool): | |||
| value = str(value) | |||
There was a problem hiding this comment.
I don't like this implementation because it will give "True" and "False" as strings. Other languages represent them differently. The encoding of booleans as strings should really be specified unambiguously so push rules can be written to work on any homeserver.
There was a problem hiding this comment.
Agreed, the below string check lowercases them but it's not explicit in any way.
There was a problem hiding this comment.
ah, well, it feels less bad then (Python's capitalisation is particularly unusual, I guess), but I suppose we still ought to make it explicit in the spec how things should be stringified if this is what we're doing.
|
Given the discussion - definitely feels like this should be an MSC! I've not got much time at the moment but can probably write something up soonish.
I'd also lean this way, string matching on booleans feels like a hack. Same for numbers/null values. |
clokep
added
the
X-Awaiting-Changes
reivilibre
added
the
Z-Spec-Blocked
For cross-linking this is MSC3758. |
|
Closing this out, the MSC3758 takes a much better approach (match the value + type). |
Quick change so boolean event fields can be evaluated in push rules (as strings). Alternative could be a different matcher type (
event_match_boolean) but that felt like overkill.Signed of by Nick Mills-Barrett nick@beeper.com
Pull Request Checklist
(run the linters)