Skip to content

Trim Async suffix on action names #7420

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
Feb 12, 2019
Merged

Trim Async suffix on action names #7420

merged 1 commit into from
Feb 12, 2019

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Feb 9, 2019

Fixes #4849

@pranavkm pranavkm requested review from rynowak and dougbu February 9, 2019 21:54
@pranavkm pranavkm added breaking-change This issue / pr will introduce a breaking change, when resolved / merged. area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Feb 9, 2019
@davidfowl
Copy link
Member

cc @BrennanConroy @halter73 We should do this in SignalR as well

dougbu
dougbu approved these changes Feb 10, 2019
View reviewed changes
src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs Outdated
/// Gets or sets a value that determines if MVC will ignore the suffix "Async" applied to
/// controller action names.
/// <para>
/// When <see langword="true"/>, MVC will strip action names of the Async suffix.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/the/an/ (action names don't always end with Async 😺)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix this

src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/AsyncActionsTests.cs Outdated
}

[Fact]
public async Task ActionCannotBeRoutedWithAsyncSuffix()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest ActionIsNotRoutedWithAsyncSuffix() because it can be routed if the convention is disabled.

@rynowak
Copy link
Member

rynowak commented Feb 10, 2019

What do you do if you want the action name to use async? [ActionName(...)]?

@dougbu
Copy link
Contributor

dougbu commented Feb 10, 2019

[ActionName(...)]?

Or, turn the new option off.

@pranavkm
Copy link
Contributor Author

Hadn't thought about ActionName. It does make sense to leave it alone if the user explicitly specified it. However, since ActionModel.ActionName is constructed accounting for ActionNameAttribute we wouldn't be able to support a convention for this. No biggie, but FYI since that would disappear in the next change

@pranavkm
Copy link
Contributor Author

🆙 📅

dougbu
dougbu approved these changes Feb 12, 2019
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Still looks good to me

src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs Outdated
/// Gets or sets a value that determines if MVC will ignore the suffix "Async" applied to
/// controller action names.
/// <para>
/// When <see langword="true"/>, MVC will strip action names of the Async suffix.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix this

src/Mvc/test/WebSites/BasicWebSite/Controllers/AsyncActionsController.cs Outdated
return Ok();
}

public IActionResult ActionReturningViewAsync()
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor but it looks strange to have a sync method named with an Async suffix.

@pranavkm pranavkm merged commit 173b2f9 into master Feb 12, 2019
@pranavkm pranavkm deleted the prkrishn/4849 branch February 12, 2019 21:30
@halter73 halter73 mentioned this pull request Feb 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu approved these changes

@rynowak rynowak Awaiting requested review from rynowak

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pranavkm @davidfowl @rynowak @dougbu