api: Add getMessageCompat helper, for servers with and without FL 120

[chrisbobbe]

We can use this to get raw Markdown content for quote-and-reply
(#116) and for the "Share" option on a message. For those, we only
care about the raw Markdown content and so could just as well have
used the raw_content field on the get-single-message response, for
servers pre-120. But...

We can also use this for #73, "Handle Zulip-internal links by
navigation", to follow /near/ links through topic/stream moves
(see implementation in zulip-mobile). For that, we'll need more than
just the message's raw Markdown.

Fixes: #140

[gnprice]

Thanks! Comments below.

[gnprice]

This isn't needed — the API calls for it only in the path.

(The way the API docs are arranged isn't super clear about this, but it's there in the orange mention of "IN PATH":

and then it's made unambiguous by the example in the curl tab.)

[gnprice]

Suggested change

	final String? rawContent; // (Marked optional for a future where this is removed)
	// final String rawContent; // deprecated; ignore

It's always safe to leave properties out of the result types, because we never have the deserialization look for unexpected properties to complain about. (Because that would risk incompatibility with future server versions.)

Generally when implementing a route binding we include properties even when we don't anticipate using them, for the reasons at https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#lazy-programming . But when the property is actually deprecated, that changes things: the ideal client in a world of ideal software would then leave the property out, so we leave it out unless there's some practical reason we currently need it.

[gnprice]

Suggested change

	final messages = response.messages;
	if (messages.isEmpty) {
	return null;
	}
	return messages.first;
	return response.messages.firstOrNull;

[gnprice]

This is probably fine in practice, but it's not the ideal behavior. I'd like to try to stick to the ideal behavior, to see if we have a pattern that makes it comfortable to do so.

The reason it's not ideal is that the ! makes it effectively an internal error in our code if the property is missing. Whereas if it is missing, that signals instead that we have a malformed JSON response from the server — one that doesn't match our expectations from the API. Ordinarily, that would cause a MalformedServerResponseException, per this bit of ApiConnection.send:

    try {
      return fromJson(json);
    } catch (e) {
      throw MalformedServerResponseException(
        routeName: routeName, httpStatus: httpStatus, data: json);
    }

So this could instead check for null and throw such an exception in that case.

In this instance, though, I think there's actually a simpler way; see next comment.

[gnprice]

Since rawContent is deprecated and we don't intend to use it, this endpoint doesn't have a remaining use case for older servers where this message property is absent.

Given that, I think we can simplify things a bit by effectively pretending that the pre-120 version of this endpoint doesn't exist. That means:

• Have Message message, unconditionally.
• Have getMessage assert that the feature level is at least 120.

(The latter step isn't strictly necessary — without it we'd get an error anyway — but could help clarify things a bit.)

Then that also means getMessageCompat could just return result.message without !.

[gnprice]

nit:

Suggested change

	return FakeApiConnection.with_((connection) async {
	return FakeApiConnection.with_(zulipFeatureLevel: 119, (connection) async {

(instead of at the end of the callback).

Dart didn't support that syntax until something like a year or two ago — previously named arguments had to come after positional arguments — which is a reason you'll often still see code not take advantage of it. But I think a case like this where the named argument is short and important for understanding the code, and the positional one is many lines long, is an excellent use of it.

[gnprice]

Seems a bit cleaner to make this a normal if/else statement, since we're not using the expression nature of the ?: operator.

[gnprice]

Let's make expectLegacy a named parameter; I think that will make these call sites easier to read, without having to refer back to the definition to remember what the boolean means and which direction it runs in.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good; merging.