Skip to content

Conversation

@pranavkm
Copy link
Contributor

Fixes #15166

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 17, 2020
@pranavkm pranavkm requested review from dougbu and javiercn August 17, 2020 23:56
@pranavkm pranavkm added this to the 5.0.0-rc1 milestone Aug 17, 2020
detail: detail,
instance: instance);
ProblemDetails problemDetails;
if (ProblemDetailsFactory == null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some precedence for having test-only code paths in Controller \ ControllerBase (e.g.

if (_controllerContext == null)
{
_controllerContext = new ControllerContext();
}
). It felt excessive to author a default implementation of the factory when it's easy to instantiate ProblemDetails \ ValidationProblemDetails in the action.

@pranavkm pranavkm changed the base branch from master to release/5.0 August 18, 2020 00:09
Invoke-Expression "& $PSScriptRoot\dotnet-install.ps1 -Architecture $arch -Version $sdkVersion -InstallDir $installDir -NoPath"
Write-Host "Installing Runtime...& $PSScriptRoot\dotnet-install.ps1 -Architecture $arch -Runtime dotnet -Version $runtimeVersion -InstallDir $installDir -NoPath"
Invoke-Expression "& $PSScriptRoot\dotnet-install.ps1 -Architecture $arch -Runtime dotnet -Version $runtimeVersion -InstallDir $installDir -NoPath"
Write-Host "InstallDotNet.ps1 complete..."
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this change show up here❔

BTW @HaoK you'll need to port #24968 to release/5.0. It missed today's branching.

Copy link
Member

Choose a reason for hiding this comment

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

Okay I can file another PR unless we want it to go in with this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

For tracking, better separate but it's up to @pranavkm if validation passes this time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry missed this. I changed the branch after I created it, let me rebase on 5.0 since this build is busted anyway

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.

LGTM

@pranavkm pranavkm merged commit 2d6fd45 into release/5.0 Aug 18, 2020
@pranavkm pranavkm deleted the prkrishn/testable-problemdetails branch August 18, 2020 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better Error Message in ControllerBase.ValidationProblem

4 participants