Skip to content

Conversation

@ThomasArdal
Copy link
Contributor

@ThomasArdal ThomasArdal commented May 14, 2019

Newline for curly braces.

Besides this, I'm not sure this example works. I've tried copying the code into a new console application and the debugger doesn't break inside the catch of the AggregateException. When awaiting a task like this, the original exception is thrown. In this case the UnauthorizedAccessException.

If doing the following, the AggregateException is thrown:

task1.Wait();

@ThomasArdal ThomasArdal requested a review from BillWagner as a code owner May 14, 2019 11:37
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving the consistency and readability of our docs @ThomasArdal We appreciate it.

I've reviewed this change, and I'll :shipit: now.

Thanks again for contributing to docs! 🎆

@BillWagner BillWagner added the ✨ 1st-time samples contributor! Indicates PRs from new contributors to the samples repository label May 14, 2019
@BillWagner BillWagner merged commit 2419e81 into dotnet:master May 14, 2019
@ThomasArdal
Copy link
Contributor Author

@BillWagner What about my comment about the sample code not working (I think, but I'm not sure)?

@BillWagner
Copy link
Member

BillWagner commented May 14, 2019

@ThomasArdal You are correct.

I'm guessing this sample was changed from using Task.Wait to using await, as part of general cleanup for best practices.

I think the best fix is to update the sample to show both catch clauses so readers can see the difference:

try
{
    await task1;
   // task1.Wait();
} catch (IndexOutOfRangeException)
{
   Console.WriteLine("caught index of out range, thrown by awaiting the task");
}
catch (AggregateException ex)
{
   Console.WriteLine("caught aggregate exception, thrown by calling task.Wait");
   // etc.
}

Do you want to submit a new PR, or should we address this fix internally?

@ThomasArdal
Copy link
Contributor Author

Internally would be fine 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ 1st-time samples contributor! Indicates PRs from new contributors to the samples repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants