Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11852027:
Improve documentation for futures. (Closed)
|
Created:
7 years, 11 months ago by floitsch Modified:
7 years, 11 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionImprove documentation for futures.
Committed: https://code.google.com/p/dart/source/detail?r=16992
Patch Set 1 #
Total comments: 27
Patch Set 2 : Address comments. #Patch Set 3 : Replace [:...:] with backquotes. #Messages
Total messages: 7 (0 generated)
https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:25: * Future<int> successor = future.then((int value) { Nit: indent this line +2 spaces https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:36: * }); It would be nice to make the first error-handling example use the recommended convention for error handling (that is, using catchError). I might even go so far as to refrain from talking about onError until the then() documentation, given how narrow its use-cases are. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:45: * var future = getFuture(); Nit: I think this whole block (and the ones below) need 2 more spaces of indentation. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:89: * A future whose value is immediately available. This is a little misleading, since the event loop is pumped before the future is completed. Same with immediateError. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:162: * Returns a new [Future] [:f:] which is completed with the result of I don't think Dartdoc's formatter understands "[Future] [:f:]"; see the current documentation on http://api.dartlang.org/docs/bleeding_edge/dart_async/Future.html. Maybe use the Markdown-style "`f`" instead? https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:168: * used directly, as the error result, otherwise it is wrapped in an Nit: "result, otherwise" -> "result. Otherwise" https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:175: * is, it forwards the error. "forwards the error to the top-level exception handler"? https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:186: * When [this] completes with a value, the value is forwarded to [this]' Nit: "[this]'" -> "[this]'s" https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:187: * successor unmodified. It's confusing that in the documentation for then() you name the successor "f" and refer to "f" rather than "the successor", whereas here you do the opposite. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:199: * Example: There should be a newline after this line, and the code blocks should be indented four spaces. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:250: * onError: (AsyncError e) { This is another place where it would be nice to use catchError to give readers a better idea of the suggested API usage.
PTAL. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:25: * Future<int> successor = future.then((int value) { On 2013/01/11 19:55:06, nweiz wrote: > Nit: indent this line +2 spaces Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:36: * }); On 2013/01/11 19:55:06, nweiz wrote: > It would be nice to make the first error-handling example use the recommended > convention for error handling (that is, using catchError). Added a catchError in the first example. > > I might even go so far as to refrain from talking about onError until the then() > documentation, given how narrow its use-cases are. The problem is, that it is otherwise very difficult to explain the semantics of Futures. I do point out several times that the preferred way is not to use the onError in [then]. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:45: * var future = getFuture(); On 2013/01/11 19:55:06, nweiz wrote: > Nit: I think this whole block (and the ones below) need 2 more spaces of > indentation. Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:89: * A future whose value is immediately available. On 2013/01/11 19:55:06, nweiz wrote: > This is a little misleading, since the event loop is pumped before the future is > completed. Same with immediateError. Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:162: * Returns a new [Future] [:f:] which is completed with the result of On 2013/01/11 19:55:06, nweiz wrote: > I don't think Dartdoc's formatter understands "[Future] [:f:]"; see the current > documentation on > http://api.dartlang.org/docs/bleeding_edge/dart_async/Future.html. Maybe use the > Markdown-style "`f`" instead? Pretty sure it does. Will update if not. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:168: * used directly, as the error result, otherwise it is wrapped in an On 2013/01/11 19:55:06, nweiz wrote: > Nit: "result, otherwise" -> "result. Otherwise" Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:175: * is, it forwards the error. On 2013/01/11 19:55:06, nweiz wrote: > "forwards the error to the top-level exception handler"? No. to [:f:]. done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:186: * When [this] completes with a value, the value is forwarded to [this]' On 2013/01/11 19:55:06, nweiz wrote: > Nit: "[this]'" -> "[this]'s" Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:187: * successor unmodified. On 2013/01/11 19:55:06, nweiz wrote: > It's confusing that in the documentation for then() you name the successor "f" > and refer to "f" rather than "the successor", whereas here you do the opposite. Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:199: * Example: On 2013/01/11 19:55:06, nweiz wrote: > There should be a newline after this line, and the code blocks should be > indented four spaces. Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:250: * onError: (AsyncError e) { On 2013/01/11 19:55:06, nweiz wrote: > This is another place where it would be nice to use catchError to give readers a > better idea of the suggested API usage. Not possible, since it would change the semantics.
lgtm https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:36: * }); On 2013/01/11 20:46:09, floitsch wrote: > On 2013/01/11 19:55:06, nweiz wrote: > > It would be nice to make the first error-handling example use the recommended > > convention for error handling (that is, using catchError). > Added a catchError in the first example. > > > > I might even go so far as to refrain from talking about onError until the > then() > > documentation, given how narrow its use-cases are. > > The problem is, that it is otherwise very difficult to explain the semantics of > Futures. > I do point out several times that the preferred way is not to use the onError in > [then]. As I mentioned offline, I think it's important to brave the difficulty and explain the semantics from the perspective of the user (a chain of individual callbacks) rather than the perspective of the implementation (each callback having implicit onValue and onError callbacks). It's fine to do that in a separate CL, though. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:162: * Returns a new [Future] [:f:] which is completed with the result of On 2013/01/11 20:46:09, floitsch wrote: > On 2013/01/11 19:55:06, nweiz wrote: > > I don't think Dartdoc's formatter understands "[Future] [:f:]"; see the > current > > documentation on > > http://api.dartlang.org/docs/bleeding_edge/dart_async/Future.html. Maybe use > the > > Markdown-style "`f`" instead? > > Pretty sure it does. Will update if not. You can see in the existing docs that it's not working. I think it conflicts with Markdown's [foo][bar] link syntax. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:250: * onError: (AsyncError e) { On 2013/01/11 20:46:09, floitsch wrote: > On 2013/01/11 19:55:06, nweiz wrote: > > This is another place where it would be nice to use catchError to give readers > a > > better idea of the suggested API usage. > > Not possible, since it would change the semantics. Not if catchError() comes before then().
https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart File sdk/lib/async/future.dart (right): https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:162: * Returns a new [Future] [:f:] which is completed with the result of On 2013/01/11 21:39:06, nweiz wrote: > On 2013/01/11 20:46:09, floitsch wrote: > > On 2013/01/11 19:55:06, nweiz wrote: > > > I don't think Dartdoc's formatter understands "[Future] [:f:]"; see the > > current > > > documentation on > > > http://api.dartlang.org/docs/bleeding_edge/dart_async/Future.html. Maybe use > > the > > > Markdown-style "`f`" instead? > > > > Pretty sure it does. Will update if not. > > You can see in the existing docs that it's not working. I think it conflicts > with Markdown's [foo][bar] link syntax. Done. https://codereview.chromium.org/11852027/diff/1/sdk/lib/async/future.dart#new... sdk/lib/async/future.dart:250: * onError: (AsyncError e) { On 2013/01/11 21:39:06, nweiz wrote: > On 2013/01/11 20:46:09, floitsch wrote: > > On 2013/01/11 19:55:06, nweiz wrote: > > > This is another place where it would be nice to use catchError to give > readers > > a > > > better idea of the suggested API usage. > > > > Not possible, since it would change the semantics. > > Not if catchError() comes before then(). point taken. For now I will leave it as is. I will revisit next week when going through it again.
Message was sent while issue was closed.
@Lasse and @Anders Please have look too.
Message was sent while issue was closed.
I read it on the API site, very nice! |
