Skip to content

Update Program.cs #36103

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 21, 2022
Merged

Update Program.cs #36103

merged 1 commit into from
Jan 21, 2022

Conversation

IEvangelist
Copy link
Member

@IEvangelist IEvangelist commented Sep 2, 2021 •
edited
Loading

I think most of the other top-level programs/templates call Run() instead of await RunAsync(). For the sake of consistency we should probably use Run.

Related to the evolution of templates with .NET 6:

/cc @DamianEdwards @bradygaster @davidfowl and @shirhatti

@IEvangelist
Update Program.cs
4f56e6c 
I think most of the other top-level programs call `Run()` instead of `await RunAsync()`. For the sake of consistency we should probably use `Run`.
@IEvangelist IEvangelist requested a review from Pilchie as a code owner September 2, 2021 18:31
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 2, 2021
@DamianEdwards
Copy link
Member

Do we know if there was a specific reason the worker template called RunAsync() already?

@IEvangelist
Copy link
Member Author

Do we know if there was a specific reason the worker template called RunAsync() already?

Hi @DamianEdwards - I'm the one who originally proposed the async version. I personally liked it better, and preferred the explicit usage of asynchrony. While I understand that the Run() does something similar to .ConfigureAwait(false).GetAwaiter().GetResult() and it's "safe" to do so, I still leaned towards liking the async main. I also like that is showcases async and await in top-level programs, which through the perspective of new developers coming to .NET it might be nice for the discoverability.

There was no other reason than my personal preferences and the original PR. For the purpose of consistency however, I think we should use Run instead (even though I prefer RunAsync). Does that make sense?

mkArtakMSFT
mkArtakMSFT approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Thanks @IEvangelist!
@rafikiassumanimsft do you want to sign-off on this too as I believe your team owns the worker template?

@pranavkm
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@pranavkm pranavkm enabled auto-merge (squash) January 21, 2022 17:43
@pranavkm pranavkm merged commit 85ac505 into main Jan 21, 2022
@pranavkm pranavkm deleted the consistent-run-invocation branch January 21, 2022 19:38
@ghost ghost added this to the 7.0-preview1 milestone Jan 21, 2022
ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022
@IEvangelist @ShreyasJejurkar
Update Program.cs (dotnet#36103)
69477f6 
I think most of the other top-level programs/templates call `Run()` instead of `await RunAsync()`. For the sake of consistency we should probably use `Run`.

Related to the evolution of templates with .NET 6:

- dotnet#33853
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkArtakMSFT mkArtakMSFT mkArtakMSFT approved these changes

@Pilchie Pilchie Awaiting requested review from Pilchie

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
7.0-preview1
Development

Successfully merging this pull request may close these issues.
4 participants
@IEvangelist @DamianEdwards @pranavkm @mkArtakMSFT