Asynchronous programming codelab: consider updates to example code

[chalin]

Moving the discussion from #2114 (comment) to this issue so that PR #2114 can contain only the original code. Copying the relevant parts of the discussion below.

Also consider that the analyzer (with the package using the pedantic analyzer options), raises issues with quite a few samples of the code, including: await_only_futures, argument_type_not_assignable, invalid_assignment, type_annotate_public_apis (which is the most common, but probably the least problematic). This should probably be addressed. In #2114, we used ignore directives to silence the analyzer.

---

Here's part of the old code that is relevant to this discussion:

void createOrderMessage () async {
  print('Awaiting user order...');
  var order = await getUserOrder();
  print('Your order is: $order');
}

Future<String> getUserOrder() {
  // Imagine that this function is more complex and slow.
  return Future.delayed(Duration(seconds: 4), () => 'Large Latte');
}

main() async {
  countSeconds(4);
  await createOrderMessage();
}

Here's the reasoning behind the changes I made, along with comments about what I think is peculiar about the original code:

1. At the start of (original) Working with futures: async and await we write:

   If [an async] ... function doesn’t explicitly return a value, then the return type is Future<void>:

   Future<void> main() async {

   So, ...

   1. Shouldn't we be following our own guidelines?
   2. Why do we given an example of attributing a return type of Future<void> to main() if we never do so in the codelab?

2. Pedagogically, I find it strange that as we work through the codelab, String createOrderMessage() starts off with a semantically meaningful name and consistent return type. Then later we repurpose the function to merely print the order message (rather than return it) and so change the signature to void createOrderMessage(). By doing so we're introducing an incongruity. One solution would be to change the function name to printOrderMessage().

3. Regardless of our choice of function name in the previous point (say, void printOrderMessage()), by using a return type of void we're again not following our own guideline (the guideline I mention in point 1), and we're not explaining why we're deviating from our own guideline.

Thoughts?

---

Btw, when createOrderMessage() is declared as void, we get the following issue reported by the analyzer:

'await' applied to 'void', which is not a 'Future'. dart(await_only_futures)

[legalcodes]

Thanks for raising this issue, happy to consider changes to this codelab!

Re: If [an async] ... function doesn’t explicitly return a value, then the return type is Future<void>, is this the relevant part of the codelab you're referencing?

[chalin]

Yes

[legalcodes]

@galeyang has kindly agreed to review notes about any UX considerations that were relevant here. Outside of that, I'm totally open to adding the type annotations you've suggested.

Does this capture the proposed changes? (Note for posterity: these screenshots are included because the code under review is currently hosted in GitHub gists, and was therefore not explicitly compared in the GitHub UI within the relevant PR.)

[chalin]

Thanks @galeyang! Thanks for the code diffs Jon, although I'm not sure how clear they are with the docregion tags. PR #2114 now contains the original code, so the simplest way to visualize the suggested code changes would be to submit a PR with the changes made (that way we could use the GitHub diff).

That being said, the main suggested code changes are:

1. Update the code so that it has no analyzer warnings when configured to use the pedantic analyzer options.
   In particular, this would mean (but not be limited to):
   • Always declaring main() with a return type.
   • Always declaring async functions returning nothing as Future<void> (not void, as is done in a few cases).
2. Don't repurpose createOrderMessage(), instead rename the function to, say, printOrderMessage(). See (2) in the original comment for details.

Other points are raised in the original comment of this issue.

Also, a minor suggested change is to write getUserOrder() as an arrow function.

[legalcodes]

Agreed that ideally we can review these changes in a GitHub PR, but since the code still lives in gists, we can't compare updates in the PR to the prior code (i.e. the diff will simply be new code, as opposed to a diff of changes to the old code), or have I missed something @chalin ?

[legalcodes]

I'm assuming you meant that once #2114 is merged we'll be able to raise the changes in a GitHub PR, which works for me. It's worth noting that we don't have an ETA on merging that PR yet, correct?

[chalin]

Agreed that ideally we can review these changes in a GitHub PR, but since the code still lives in gists, ...

Oh, I'm sorry if it wasn't clear. I just added #2114 (comment) to clarify that #2114 now uses the same code as the Gists, modulo running dartfmt. So the code in the code blocks of async-await.md corresponds to the Gist code. (FYI, this is the main commit that reverts code changes: ed3930e.)

I'm assuming you meant that once #2114 is merged ...

AFAIK, it is possible to submit a PR over a PR (and then to view the new PR's delta, just look at the new PR's single commit diff).

[galeyang]

Re: @chalin's initial comment

#1:I'm trying to remember our discussion around the return type declaration before main() and found previous comments between @legalcodes, @kwalrath and me. Basically I found it confusing if we only add Future<void> before main() in some examples but not all examples, especially when we didn't explain why in that version. Kathy and Jon talked about It seems sort of odd to show examples with Future<void> main() everywhere, but it also seems odd to do it in some places but not others. Eventually we added a blue note to explain that return type declaration being OK to leave off.

This note was initially put at the end, a user (via survey) suggested us to move it upfront (issue). I'm fine with either having Future<void> consistently or leave it as it is now and ensure we explain why sometimes they are different.

#2: I'm fine with this!
#3: Good point. I actually don't know why it is not Future<void> but void.

[galeyang]

I just found in the Effective Dart, it has recommendations

[kwalrath]

I think that recommendation was pointed at fully typed older code that used either Future or Future<Null>. Taken literally, it doesn't seem to apply to main because main isn't a member (afaik).

That said, I think the pedantic package might now complain if we don't type main. (Not sure about that. @chalin, do you happen to know?)

[chalin]

the pedantic package might now complain if we don't type main.

Yes:

Which is why the Async codelab code in #2114 is sprinkled with ignore directives :).

[legalcodes]

Summing up (referencing @chalin 's original proposal) it sounds like we're all OK with:
(1) Declaring main() with a return type for all example code in this codelab.
(2) Declaring async functions that return nothing as Future<void>.
(3) Rename createOrderMessage() to printOrderMessage().

So these changes should go in!

Does anyone want to weigh in on using arrow functions as proposed? @kwalrath ?

[chalin]

Do we generally agree that codelab, tutorial and pretty much all example code should be free of analyzer issues when configured with the pedantic options?

[chalin]

(Unless, of course, it is meant to, e.g.: illustrate errors, or be an incomplete starting point for an exercise.)

[legalcodes]

Speaking for myself: yes, absolutely. Thanks Patrice!

[natebosch]

+1 for using Future<void> as a return type in any place where we will be using an await with it.

void as a return type is OK only if callers should never await it, which I don't think comes up in this codelab.

+1 for changing the name between create and print depending on the behavior, return vs print.

Also, see #2134

We should rename all get* methods, and ideally make some of them getters.

[chalin]

There are still some points from this issue that haven't been addressed. Reopening.

[legalcodes]

@chalin Can you specify which points from this issue haven't been addressed?

[chalin]

Can you specify which points ...

• https://github.com/dart-lang/site-www/pull/2137/files#r350924276, which you know about already.
• https://github.com/dart-lang/site-www/pull/2137/files#r351422017
• Closes #2128 Reinstate createOrderMessage() in cases lacking print statements #2141 (comment), the return type for both the sync and async version of main().