model: Use enum for Message.flags[] #298
Conversation
…type In practice the returned map has always had string keys, but its `runtimeType` has apparently been _Map<dynamic, Object?>. Make it be Map<String, dynamic> instead. If you have an instance of one of our data classes, and one of its fields is represented in the `toJson` form as a string-keyed Map, then now the instance can pass through `deepToJson` and back through the class's `fromJson` factory without having a runtime type-casting error in `fromJson`.
chrisbobbe
force-pushed
the
pr-flags-set-enum
branch
from
1ecd7dd to
76855ce
Compare
|
(Updated PR description to acknowledge commits cherry-picked from other PRs.) |
lib/api/model/model.dart
Outdated
| static Object? _flagsToJson(Set<Flag> value) { | ||
| return value.map((flag) => flag.toJson()).toList(); |
There was a problem hiding this comment.
The toJson call isn't needed here, right? In that our convention generally is that toJson isn't recursive, and the recursion is left up to jsonEncode or to our tests' deepToJson.
(And then at that point this method no longer does anything and can be dropped.)
There was a problem hiding this comment.
Huh, interesting, right. I think that means that the whole _flagsToJson just isn't needed.
lib/api/model/model.dart
Outdated
| @@ -305,6 +315,32 @@ sealed class Message { | |||
| Map<String, dynamic> toJson(); | |||
| } | |||
|
|
|||
| /// As in [Message.flags]. | |||
| @JsonEnum(fieldRename: FieldRename.snake, alwaysCreate: true) | |||
| enum Flag { | |||
There was a problem hiding this comment.
How about MessageFlag? I feel like Flag, as a public name visible across our codebase, is nonspecific enough that it sounds like it could be a lot of things depending on the context.
lib/api/model/events.dart
Outdated
| @@ -341,7 +341,7 @@ class UpdateMessageEvent extends Event { | |||
| final bool? renderingOnly; // TODO(server-5) | |||
| final int messageId; | |||
| final List<int> messageIds; | |||
| final List<String> flags; // TODO enum | |||
| final Set<Flag> flags; | |||
There was a problem hiding this comment.
model: Make `flags` on Message and UpdateMessageEvent a Set instead of List
This will enable more efficient checking of whether a message has a
certain flag.
I think this is actually a case where a List is likely to be more efficient than a Set.
The reason is that these lists will always be short, and almost always be very short. There are 7 different flags now; the list might grow occasionally but will probably never be much more than 10; so the list will never be longer than that. And most of the flags will apply to a small fraction of messages; probably over 90% of the lists of flags will be either just [Flag.read], or empty.
When the list/set is small, checking membership by just looping through wins for speed because of its simplicity — there's just so little work to do. In particular when the length is 0 or 1, the loop is basically impossible to beat.
There was a problem hiding this comment.
If we wanted to really optimize this — for speed and also for space — the way to do it would be with a single int that contains flag bits. That might actually turn out to be worthwhile, given that messages are something we can have a lot of. But we can leave it out at this stage.
There was a problem hiding this comment.
Interesting. Sure, we can keep it as a List.
It would have been nice to be able to annotate [MessageFlag.unknown] as the "unknown" value inline, right there in the enum definition. If the enum described a field directly on the Message class, we could at least have annotated that field with @jsonkey(unknownEnumValue: MessageFlag.unknown) and that would be nice and compact. But the field is a List<MessageFlag>, not a MessageFlag, so we have to verbosely write out `fromJson` for it, ah, well.
chrisbobbe
force-pushed
the
pr-flags-set-enum
branch
from
76855ce to
9829f60
Compare
chrisbobbe
changed the title
|
Thanks for the review! Revision pushed. I've updated the PR title so it doesn't say we're using a Set anymore. |
|
Thanks! Looks good; merging. |
This contains commits cherry-picked from other PRs:
From my f59da2a / #286:
From @sirpengi's 00bc224 / #287 (thanks @sirpengi!):
Related: #253