api: Document, and sweep for, our practice of making constructor arguments explicit

README.md

@@ -126,6 +126,15 @@ to keep the generated files up to date:
 $ dart run build_runner watch --delete-conflicting-outputs
 ```
 
+In our API types, constructors should generally avoid default values for
+their parameters, even `null`.  This means writing e.g. `required this.foo`
+rather than just `this.foo`, even when `foo` is nullable.
+This is because it's common in the Zulip API for a null or missing value
+to be quite salient in meaning, and not a boring value appropriate for a
+default, so that it's best to ensure callers make an explicit choice.
+If passing explicit values in tests is cumbersome, a factory function
+in `test/example_data.dart` is an appropriate way to share defaults.
+
 
 ### Upgrading Flutter
 

lib/api/model/events.dart

@@ -249,6 +249,9 @@ class RealmUserUpdateEvent extends RealmUserEvent {
   RealmUserUpdateEvent({
     required super.id,
     required this.userId,
+    // Unlike in most of the API bindings, we leave these constructor arguments
+    // optional.  That's because in these events only one or a handful of these
+    // will appear in a given event value.
     this.fullName,
     this.avatarUrl,
     this.avatarVersion,
@@ -384,23 +387,23 @@ class UpdateMessageEvent extends Event {
 
   UpdateMessageEvent({
     required super.id,
-    this.userId,
-    this.renderingOnly,
+    required this.userId,
+    required this.renderingOnly,
     required this.messageId,
     required this.messageIds,
     required this.flags,
-    this.editTimestamp,
-    this.streamName,
-    this.streamId,
-    this.newStreamId,
-    this.propagateMode,
-    this.origSubject,
-    this.subject,
-    this.origContent,
-    this.origRenderedContent,
-    this.content,
-    this.renderedContent,
-    this.isMeMessage,
+    required this.editTimestamp,
+    required this.streamName,
+    required this.streamId,
+    required this.newStreamId,
+    required this.propagateMode,
+    required this.origSubject,
+    required this.subject,
+    required this.origContent,
+    required this.origRenderedContent,
+    required this.content,
+    required this.renderedContent,
+    required this.isMeMessage,
   });
 
   factory UpdateMessageEvent.fromJson(Map<String, dynamic> json) =>
@@ -434,8 +437,8 @@ class DeleteMessageEvent extends Event {
     required super.id,
     required this.messageIds,
     required this.messageType,
-    this.streamId,
-    this.topic,
+    required this.streamId,
+    required this.topic,
   });
 
   factory DeleteMessageEvent.fromJson(Map<String, dynamic> json) {

lib/api/model/initial_snapshot.dart

@@ -73,11 +73,11 @@ class InitialSnapshot {
   }
 
   InitialSnapshot({
-    this.queueId,
+    required this.queueId,
     required this.lastEventId,
     required this.zulipFeatureLevel,
     required this.zulipVersion,
-    this.zulipMergeBase,
+    required this.zulipMergeBase,
     required this.alertWords,
     required this.customProfileFields,
     required this.recentPrivateConversations,

lib/api/model/model.dart

@@ -142,7 +142,7 @@ class User {
   Map<int, ProfileFieldUserData>? profileData;
 
   @JsonKey(readValue: _readIsSystemBot)
-  bool? isSystemBot; // TODO(server-5)
+  bool isSystemBot;
 
   static Map<String, dynamic>? _readProfileData(Map json, String key) {
     final value = (json[key] as Map<String, dynamic>?);
@@ -153,8 +153,12 @@ class User {
     return (value != null && value.isNotEmpty) ? value : null;
   }
 
-  static bool? _readIsSystemBot(Map json, String key) {
-    return json[key] ?? json['is_cross_realm_bot'];
+  static bool _readIsSystemBot(Map json, String key) {
+    // This field is absent in `realm_users` and `realm_non_active_users`,
+    // which contain no system bots; it's present in `cross_realm_bots`.
+    return json[key]
+        ?? json['is_cross_realm_bot'] // TODO(server-5): renamed to `is_system_bot`
+        ?? false;
   }
 
   User({
@@ -169,14 +173,14 @@ class User {
     required this.isGuest,
     required this.isBillingAdmin,
     required this.isBot,
-    this.botType,
-    this.botOwnerId,
+    required this.botType,
+    required this.botOwnerId,
     required this.role,
     required this.timezone,
     required this.avatarUrl,
     required this.avatarVersion,
-    this.profileData,
-    this.isSystemBot,
+    required this.profileData,
+    required this.isSystemBot,
   });
 
   factory User.fromJson(Map<String, dynamic> json) => _$UserFromJson(json);
@@ -190,7 +194,14 @@ class ProfileFieldUserData {
   final String value;
   final String? renderedValue;
 
-  ProfileFieldUserData({required this.value, this.renderedValue});
+  ProfileFieldUserData({
+    required this.value,
+    // Unlike in most of the API bindings, we leave this constructor argument
+    // optional.  That's because for most types of custom profile fields,
+    // this property is always indeed absent, and because this constructor is
+    // otherwise convenient to write many calls to in our test code.
+    this.renderedValue,
+  });
 
   factory ProfileFieldUserData.fromJson(Map<String, dynamic> json) =>
     _$ProfileFieldUserDataFromJson(json);
@@ -317,22 +328,22 @@ class Subscription {
     required this.renderedDescription,
     required this.dateCreated,
     required this.inviteOnly,
-    this.desktopNotifications,
-    this.emailNotifications,
-    this.wildcardMentionsNotify,
-    this.pushNotifications,
-    this.audibleNotifications,
+    required this.desktopNotifications,
+    required this.emailNotifications,
+    required this.wildcardMentionsNotify,
+    required this.pushNotifications,
+    required this.audibleNotifications,
     required this.pinToTop,
     required this.emailAddress,
     required this.isMuted,
-    this.isWebPublic,
+    required this.isWebPublic,
     required this.color,
     required this.streamPostPolicy,
-    this.messageRetentionDays,
+    required this.messageRetentionDays,
     required this.historyPublicToSubscribers,
-    this.firstMessageId,
-    this.streamWeeklyTraffic,
-    this.canRemoveSubscribersGroupId,
+    required this.firstMessageId,
+    required this.streamWeeklyTraffic,
+    required this.canRemoveSubscribersGroupId,
   });
 
   factory Subscription.fromJson(Map<String, dynamic> json) =>
@@ -395,7 +406,7 @@ sealed class Message {
     required this.contentType,
     required this.id,
     required this.isMeMessage,
-    this.lastEditTimestamp,
+    required this.lastEditTimestamp,
     required this.reactions,
     required this.recipientId,
     required this.senderEmail,
@@ -405,8 +416,8 @@ sealed class Message {
     required this.subject,
     required this.timestamp,
     required this.flags,
-    this.matchContent,
-    this.matchSubject,
+    required this.matchContent,
+    required this.matchSubject,
   });
 
   factory Message.fromJson(Map<String, dynamic> json) {
@@ -460,7 +471,7 @@ class StreamMessage extends Message {
     required super.contentType,
     required super.id,
     required super.isMeMessage,
-    super.lastEditTimestamp,
+    required super.lastEditTimestamp,
     required super.reactions,
     required super.recipientId,
     required super.senderEmail,
@@ -470,8 +481,8 @@ class StreamMessage extends Message {
     required super.subject,
     required super.timestamp,
     required super.flags,
-    super.matchContent,
-    super.matchSubject,
+    required super.matchContent,
+    required super.matchSubject,
     required this.displayRecipient,
     required this.streamId,
   });
@@ -562,7 +573,7 @@ class DmMessage extends Message {
     required super.contentType,
     required super.id,
     required super.isMeMessage,
-    super.lastEditTimestamp,
+    required super.lastEditTimestamp,
     required super.reactions,
     required super.recipientId,
     required super.senderEmail,
@@ -572,8 +583,8 @@ class DmMessage extends Message {
     required super.subject,
     required super.timestamp,
     required super.flags,
-    super.matchContent,
-    super.matchSubject,
+    required super.matchContent,
+    required super.matchSubject,
     required this.displayRecipient,
   });
 

lib/api/route/account.dart

@@ -32,7 +32,7 @@ class FetchApiKeyResult {
   FetchApiKeyResult({
     required this.apiKey,
     required this.email,
-    this.userId,
+    required this.userId,
   });
 
   factory FetchApiKeyResult.fromJson(Map<String, dynamic> json) =>

lib/api/route/events.dart

@@ -41,7 +41,7 @@ class GetEventsResult {
 
   GetEventsResult({
     required this.events,
-    this.queueId,
+    required this.queueId,
   });
 
   factory GetEventsResult.fromJson(Map<String, dynamic> json) =>

lib/api/route/messages.dart

@@ -226,11 +226,9 @@ class DmDestination extends MessageDestination {
 @JsonSerializable(fieldRename: FieldRename.snake)
 class SendMessageResult {
   final int id;
-  final String? deliverAt;
 
   SendMessageResult({
     required this.id,
-    this.deliverAt,
   });
 
   factory SendMessageResult.fromJson(Map<String, dynamic> json) =>

lib/api/route/realm.dart

@@ -52,7 +52,7 @@ class GetServerSettingsResult {
     required this.authenticationMethods,
     required this.zulipFeatureLevel,
     required this.zulipVersion,
-    this.zulipMergeBase,
+    required this.zulipMergeBase,
     required this.pushNotificationsEnabled,
     required this.isIncompatible,
     required this.emailAuthEnabled,
@@ -61,7 +61,7 @@ class GetServerSettingsResult {
     required this.realmName,
     required this.realmIcon,
     required this.realmDescription,
-    this.realmWebPublicAccessEnabled,
+    required this.realmWebPublicAccessEnabled,
   });
 
   factory GetServerSettingsResult.fromJson(Map<String, dynamic> json) =>

test/api/model/model_test.dart

@@ -59,9 +59,9 @@ void main() {
     });
 
     test('is_system_bot', () {
-      check(mkUser({}).isSystemBot).isNull();
-      check(mkUser({'is_cross_realm_bot': true}).isSystemBot).equals(true);
-      check(mkUser({'is_system_bot': true}).isSystemBot).equals(true);
+      check(mkUser({}).isSystemBot).isFalse();
+      check(mkUser({'is_cross_realm_bot': true}).isSystemBot).isTrue();
+      check(mkUser({'is_system_bot': true}).isSystemBot).isTrue();
     });
   });
 

test/api/route/route_checks.dart

@@ -3,7 +3,6 @@ import 'package:zulip/api/route/messages.dart';
 
 extension SendMessageResultChecks on Subject<SendMessageResult> {
   Subject<int> get id => has((e) => e.id, 'id');
-  Subject<String?> get deliverAt => has((e) => e.deliverAt, 'deliverAt');
 }
 
 // TODO add similar extensions for other classes in api/route/*.dart

test/example_data.dart

@@ -42,11 +42,14 @@ User user({
     isGuest: false,
     isBillingAdmin: false,
     isBot: false,
+    botType: null,
+    botOwnerId: null,
     role: UserRole.member,
     timezone: 'UTC',
     avatarUrl: avatarUrl,
     avatarVersion: 0,
     profileData: profileData,
+    isSystemBot: false,
   );
 }
 
@@ -279,6 +282,40 @@ const _unreadMsgs = unreadMsgs;
 // Events.
 //
 
+UpdateMessageEvent updateMessageEditEvent(
+  Message origMessage, {
+  int? userId = -1, // null means null; default is [selfUser.userId]
+  bool? renderingOnly = false,
+  int? messageId,
+  List<MessageFlag>? flags,
+  int? editTimestamp,
+  String? streamName,
+  String? renderedContent,
+  bool isMeMessage = false,
+}) {
+  messageId ??= origMessage.id;
+  return UpdateMessageEvent(
+    id: 0,
+    userId: userId == -1 ? selfUser.userId : userId,
+    renderingOnly: renderingOnly,
+    messageId: messageId,
+    messageIds: [messageId],
+    flags: flags ?? origMessage.flags,
+    editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp
+    streamName: streamName,
+    streamId: origMessage is StreamMessage ? origMessage.streamId : null,
+    newStreamId: null,
+    propagateMode: null,
+    origSubject: null,
+    subject: null,
+    origContent: 'some probably-mismatched old Markdown',
+    origRenderedContent: origMessage.content,
+    content: 'some probably-mismatched new Markdown',
+    renderedContent: renderedContent ?? origMessage.content,
+    isMeMessage: isMeMessage,
+  );
+}
+
 UpdateMessageFlagsRemoveEvent updateMessageFlagsRemoveEvent(
   MessageFlag flag,
   Iterable<Message> messages, {