-
Notifications
You must be signed in to change notification settings - Fork 350
initial_snapshot: Accept unknown "emojiset" values #1983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Let's make a new enum value |
df1e800 to
9382645
Compare
|
Thank you @chrisbobbe for the review, introduced a new enum value |
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Small comments below.
| 'presence_enabled': true, | ||
| }); | ||
|
|
||
| // Verify unknown emojiset defaults to Emojiset.unknow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Emojiset.unknown
| final settings = UserSettings.fromJson({ | ||
| 'twenty_four_hour_time': true, | ||
| 'display_emoji_reaction_users': true, | ||
| 'emojiset': unknownValue, | ||
| 'presence_enabled': true, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This .fromJson call will fail and need to be debugged each time we add a field to UserSettings.
Can you add a userSettings helper function in test/example_data.dart, on the pattern of similar helpers (like streamMessage or initialSnapshot), then I think this can just be
| final settings = UserSettings.fromJson({ | |
| 'twenty_four_hour_time': true, | |
| 'display_emoji_reaction_users': true, | |
| 'emojiset': unknownValue, | |
| 'presence_enabled': true, | |
| }); | |
| final json = eg.userSettings().toJson()..['emojiset'] = unknownValue; | |
| final settings = UserSettings.fromJson(json); |
9382645 to
1896109
Compare
1896109 to
59cd993
Compare
|
Thank you @chrisbobbe for the Suggestion, have pushed the changes. PTAL. |
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just small comments below.
Also something I just noticed on this round: for the commit-message summary line, let's say
api: Accept unknown "emojiset" values in initial snapshot
| return UserSettings( | ||
| twentyFourHourTime: twentyFourHourTime ?? TwentyFourHourTimeMode.twentyFourHour, | ||
| displayEmojiReactionUsers: displayEmojiReactionUsers ?? true, | ||
| emojiset: emojiset ?? Emojiset.unknown , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should aim to make the defaults boring and representative of real data, and I think Emojiset.google would be better for that than Emojiset.unknown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this helper can deduplicate some code in the initialSnapshot helper just below this.
Just below the userSettings function declaration, you can say
const _userSettings = userSettings;then in initialSnapshot in choosing arguments for the InitialSnapshot constructor, you can say:
userSettings: userSettings ?? _userSettings(),
Fixes #1981.