-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Future#handleException is too long! #3007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
This comment was originally written by [email protected] I was thinking about the name, came up with 'but', 'thrown' or 'caught', but none of them is particularly nice. 'except' FTW! |
This comment was originally written by @seaneagan How about: defend I also like 'recover' except it makes it sound like the exception already happened when it is called. |
except() works for me. cc @sigmundch. |
How about catch() ? |
This comment was originally written by [email protected] else() ? |
I hope we can find something that is easy to understand also it we use it alone (without 'then'). In particular, 'except' reads nice when using a fluent API (.then().except()), but it's harder to understand in code like this: We can't use 'else' and 'catch' because they are reserved words in Dart. Here are some other proposals I've heard:
Any other ideas? cc @munificent. |
This comment was originally written by [email protected] Personally, I like passing the error callback as a second (optional) argument. From my experience writing something quite similar in another language, we actually ended up with a third (optional) 'finally' callback. So: future.then((v) {}, (e) {}, () {}) -Ross |
I worry that this is confusing because that pattern is used for event handlers elsewhere.
That works, but then you can't really type the callback's argument in a useful way.
I like this. Maybe name it "except": file.readText().then((text) { |
This comment was originally written by [email protected]
I don't think 'then' is particularly easy to understand - to make it obvious what the callback is for, it would need to be 'onResult' or 'handleResult'. I agree that an optional 'except' parameter would be an improvement over what we have now, but I think renaming handleException is still necessary and (once we have cascades) sufficient. |
This comment was originally written by @seaneagan 'except' doesn't make sense to me. It has various forms: Preposition: Not including; other than. Conjunction: Used before a statement that forms an exception to one just made: "I didn't tell him anything, except that I needed the money". Verb: Specify as not included in a category or group; exclude. ... and none of these seem to match the idea of registering an exception handler. 'catch' is perfect, if only it weren't a keyword, :) I like passing both success and error handlers to the same method, one nit though is that passing two function literals to the same method can make it hard to tell where one starts and the other begins, but that can mostly be avoided with an idiomatic line break style. It would nice to stick to verbs for methods, some alternatives to 'then' would be: schedule |
This comment was originally written by [email protected]
C# Tasks use 'ContinueWith' which is a bit long but perhaps 'continue' is a possibility |
This comment was originally written by [email protected] An alternative is unifying the success and failure callbacks: |
This comment was originally written by [email protected] +1 for something along the lines suggested in #3047. It would reduce the number of callbacks, the familiar form of try-catch could be used, including finally, and wouldn't make it necessary to remind people to use an alternative pattern all the time. |
As part of the Future redesign, the members where renamed accordingly: .handleException -> .catchError Added Fixed label. |
New commits in this revision: git log --format=%C(auto) %h %s cafadd17ba285dad9ebe6d34c617bc0d70d096b3..98a01e1cdbf441f030f1be76417860b6b372663d ✓ cafadd17 Don't use null safety (yet) (#3112) 98a01e1c Store credentials in a config directory (#3092) 4ee280b7 Fixed messages (#3110) bbdac802 Third party hosted pub registry authentication (#3007) b1bedc53 `--examples` for get/upgrade/downgrade/add/remove (#2857) Change-Id: Ibf6ffe33820c97114958c1564a7c22a46ff73d9c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/213348 Commit-Queue: Sigurd Meldgaard <[email protected]> Reviewed-by: Jonas Jensen <[email protected]>
This issue was originally filed by [email protected]
This may seem trivial, but this method name really bugs me:
* it's inconsistent with then(), which should rather be handleResult()
* its long name (especially by contrast with 'then') suggests that it's somewhat obscure, when in fact it should usually be called on every Future.
What about
getFooAsync().then(...).except(...)?
The text was updated successfully, but these errors were encountered: