Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562703002:
Add `FutureOr<T>` class to `dart:async`. (Closed)
|
Created:
4 years ago by floitsch Modified:
4 years ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd `FutureOr<T>` class to `dart:async`.
`FutureOr<T>` is a magical class, but having a concrete implementation
makes it easier for many tools (like dartdoc, code navigation, ...) to
deal with it.
Fixes #28029
BUG= http://dartbug.com/28029
R=lrn@google.com
Committed: https://github.com/dart-lang/sdk/commit/6a966d2914b713557ea899db5937154c514c3818
Patch Set 1 #Patch Set 2 : Reword. #
Total comments: 13
Patch Set 3 : Address comments. #
Total comments: 4
Patch Set 4 : Address comments. #Messages
Total messages: 10 (3 generated)
floitsch@google.com changed reviewers: + lrn@google.com
lgtm https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:7: /// The union of `Future<T>` and `T`. I would save the "union" word to the advanced section. Maybe just: "A type representing either a `Future<T>` or a `T`." https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:9: /// This is a magic class that is recognized by the Dart tools. It is I wouldn't actually use the word "magic" in writing. Maybe something like: This class declaration represents the actual generic `FutureOr` type, which isn't itself a class type. Dart tools will treat references to this `FutureOr` class as denoting the corresponding actual future-or-value type. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:12: /// This class can not be implemented or extended. or mixed in. The spec language is (e.g.): > It is a compile-time error for any class other than \code{int} and \code{double} to extend, mix in or implement \code{num}. so maybe something like: > It's a compile-time error for any class to extend, mix in or implement \code{FutureOr}. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:27: /// Since `FutureOr<Object>` is the union of `Future<Object>` and `Object`, it "the union" isn't defined anywhere, so it's not clear what it means. Maybe: The `FutureOr<int>` type is actually the "type union" of the types `int` and `Future<int>`. This type union is defined in such a way that `FutureOr<Object>` is both a super- and sub-type of `Object` (sub-type because `Object` is one of the types of the union, super-type because `Object` is a super-type of both of the types of the union). Together it means that `FutureOr<Object>` is equivalent to `Object`. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:32: /// `FutureOr<FutureOr<Object>>`. (and equivalent to Object) The more interesting example is: FutureOr<Future<Object>> == Future<Object> https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:34: // Private constructor, so that it is not subclassable and instantiable. or mixin'able, as if that was a word. For even more breakage for anyone trying to use the class, you can make it a factory constructor which throws (no generative constructor at all), and make the class abstract. Still doesn't block the interface.
brianwilkerson@google.com changed reviewers: + brianwilkerson@google.com
https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:12: /// This class can not be implemented or extended. nit: "can not" --> "cannot" nit: or mixed in? (maybe "implemented" implies that; I usually make both "extended" and "mixed in" explicit for clarity)
https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:7: /// The union of `Future<T>` and `T`. On 2016/12/08 15:14:00, Lasse Reichstein Nielsen wrote: > I would save the "union" word to the advanced section. > Maybe just: > "A type representing either a `Future<T>` or a `T`." That wording doesn't work for me. If read the wrong way, it can only represent either or, but not both at the same time. Changed to `A type representing values that are either `Future<T>` or `T`. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:9: /// This is a magic class that is recognized by the Dart tools. It is On 2016/12/08 15:14:00, Lasse Reichstein Nielsen wrote: > I wouldn't actually use the word "magic" in writing. > Maybe something like: > > This class declaration represents the actual generic `FutureOr` type, > which isn't itself a class type. Dart tools will treat references to this > `FutureOr` class as denoting the corresponding actual future-or-value type. > > Changed. PTAL. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:12: /// This class can not be implemented or extended. On 2016/12/08 15:14:00, Lasse Reichstein Nielsen wrote: > or mixed in. > > The spec language is (e.g.): > > It is a compile-time error for any class other than \code{int} and > \code{double} to extend, mix in or implement \code{num}. > so maybe something like: > > It's a compile-time error for any class to extend, mix in or implement > \code{FutureOr}. Done. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:27: /// Since `FutureOr<Object>` is the union of `Future<Object>` and `Object`, it On 2016/12/08 15:14:00, Lasse Reichstein Nielsen wrote: > "the union" isn't defined anywhere, so it's not clear what it means. > Maybe: > > The `FutureOr<int>` type is actually the "type union" of the types `int` and > `Future<int>`. This type union is defined in such a way that `FutureOr<Object>` > is both a super- and sub-type of `Object` (sub-type because `Object` is one of > the types of the union, super-type because `Object` is a super-type of both of > the types of the union). Together it means that `FutureOr<Object>` is equivalent > to `Object`. Done. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:32: /// `FutureOr<FutureOr<Object>>`. On 2016/12/08 15:14:00, Lasse Reichstein Nielsen wrote: > (and equivalent to Object) > > The more interesting example is: > FutureOr<Future<Object>> == Future<Object> Done. https://codereview.chromium.org/2562703002/diff/20001/sdk/lib/async/future.da... sdk/lib/async/future.dart:34: // Private constructor, so that it is not subclassable and instantiable. On 2016/12/08 15:14:00, Lasse Reichstein Nielsen wrote: > or mixin'able, as if that was a word. > > For even more breakage for anyone trying to use the class, you can make it a > factory constructor which throws (no generative constructor at all), and make > the class abstract. Still doesn't block the interface. Done.
lgtm https://codereview.chromium.org/2562703002/diff/40001/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/2562703002/diff/40001/sdk/lib/async/future.da... sdk/lib/async/future.dart:10: /// future-or-value type. References to this class are resolved to the Technically (ob-xkcd 1475) it's not a *type*, it's a *type constructor*, or as we normally say, a "generic type". It won't be a type until you apply a type argument. https://codereview.chromium.org/2562703002/diff/40001/sdk/lib/async/future.da... sdk/lib/async/future.dart:40: // Private constructor, so that it is not subclassable, mixable, or Silly me, I forgot that a factory constructor doesn't actually prevent being used as a mixin any more. :( So it's probably better to drop the "factory" again, but keep the throwing.
https://codereview.chromium.org/2562703002/diff/40001/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/2562703002/diff/40001/sdk/lib/async/future.da... sdk/lib/async/future.dart:10: /// future-or-value type. References to this class are resolved to the On 2016/12/08 16:46:52, Lasse Reichstein Nielsen wrote: > Technically (ob-xkcd 1475) it's not a *type*, it's a *type constructor*, or as > we normally say, a "generic type". It won't be a type until you apply a type > argument. Done. https://codereview.chromium.org/2562703002/diff/40001/sdk/lib/async/future.da... sdk/lib/async/future.dart:40: // Private constructor, so that it is not subclassable, mixable, or On 2016/12/08 16:46:52, Lasse Reichstein Nielsen wrote: > Silly me, I forgot that a factory constructor doesn't actually prevent being > used as a mixin any more. :( > So it's probably better to drop the "factory" again, but keep the throwing. Done.
Description was changed from ========== Add `FutureOr<T>` class to `dart:async`. `FutureOr<T>` is a magical class, but having a concrete implementation makes it easier for many tools (like dartdoc, code navigation, ...) to deal with it. Fixes #28029 BUG= http://dartbug.com/28029 ========== to ========== Add `FutureOr<T>` class to `dart:async`. `FutureOr<T>` is a magical class, but having a concrete implementation makes it easier for many tools (like dartdoc, code navigation, ...) to deal with it. Fixes #28029 BUG= http://dartbug.com/28029 R=lrn@google.com Committed: https://github.com/dart-lang/sdk/commit/6a966d2914b713557ea899db5937154c514c3818 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 6a966d2914b713557ea899db5937154c514c3818 (presubmit successful). |
