api: Carry zulipFeatureLevel on ApiConnection, and use it for sending "direct" and "dm" #159
Conversation
As is, the `finally` block gets run before the future returned by the callback has completed. I believe the underlying issue here is this one in the Dart SDK: dart-lang/sdk#44395 The fix is to await the future and return the result of that, rather than returning the future directly. The Dart language folks (or at least some of them) hope to eventually deal with this by making the old code here a type error, requiring the value passed to `return` to have type T rather than Future<T>: dart-lang/language#870
This will help when we add more complexity to this route binding and correspondingly more tests.
These should suffice to cover all the logic in this route binding.
This doesn't yet do anything, but it provides the infrastructure for taking care of zulip#146: it sets us up to have individual route bindings condition their behavior on the Zulip feature level as appropriate.
| /// Instead, call [resolve] and use the object it returns. | ||
| class ApiNarrowDm extends ApiNarrowElement { | ||
| @override String get operator => 'pm-with'; // TODO(#146): use 'dm' where possible | ||
| @override String get operator { |
There was a problem hiding this comment.
This sort of thing was why I was asking about the dart format. I see both this style and the more traditional style throughout the codebase.
@override
String get operator {}
There was a problem hiding this comment.
Yeah, for the classes in this file (before this PR, anyway), they're all so short and simple and parallel to each other that I felt the one-line form like
@override String get operator => 'topic';was helpful for reading, by helping make the parallelism easy to see.
If we did adopt automated formatting, though, I wouldn't really mind these going into multi-line form as a consequence of that.
At a quick grep (git grep '@override '), I'm only finding one other spot in the codebase where we use the one-line form:
class UriConverter extends TypeConverter<Uri, String> {
const UriConverter();
@override String toSql(Uri value) => value.toString();
@override Uri fromSql(String fromDb) => Uri.parse(fromDb);
}The idea there is similar, and again it's not something I'd particularly regret going away if we adopted auto-formatting.
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, LGTM! Please merge at will.
| : FakeApiConnection(realmUrl: realmUrl); | ||
| try { | ||
| return fn(connection); | ||
| return await fn(connection); |
There was a problem hiding this comment.
api test: Fix with_ to await callback first, then close
As is, the `finally` block gets run before the future returned by
the callback has completed. I believe the underlying issue here
is this one in the Dart SDK:
https://github.com/dart-lang/sdk/issues/44395
The fix is to await the future and return the result of that,
rather than returning the future directly.
The Dart language folks (or at least some of them) hope to
eventually deal with this by making the old code here a type error,
requiring the value passed to `return` to have type T rather
than Future<T>:
https://github.com/dart-lang/language/issues/870
Interesting; yeah. I think there's a "gotcha" like this in JavaScript, too.
3 participants
Fixes: #146
In #146 I listed these two spots where we'd want to use this capability once we added it, and mentioned that there might be others. On scanning through a grep for
TODO.server, though, these are the only such places that I find.