-
Notifications
You must be signed in to change notification settings - Fork 196
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.
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
Use Basic.Reference.Assemblies in MS.CA.Razor.Test #10877
Conversation
@@ -13,7 +13,6 @@ | |||
<ItemGroup> | |||
<ProjectReference Include="..\..\src\Microsoft.CodeAnalysis.Razor.Workspaces\Microsoft.CodeAnalysis.Razor.Workspaces.csproj" /> | |||
<ProjectReference Include="..\Microsoft.AspNetCore.Razor.Test.Common.Tooling\Microsoft.AspNetCore.Razor.Test.Common.Tooling.csproj" /> | |||
<ProjectReference Include="..\Microsoft.AspNetCore.Razor.Test.MvcShim\Microsoft.AspNetCore.Razor.Test.MvcShim.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Can the tooling test shims all be deleted now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't notice I'm in the tooling layer here. It looks like even this line could have been removed independently of this PR.
Can the tooling test shims all be deleted now?
In tooling I see the shims being used in testapps/
which seem to be used by benchmarks. However it seems that the benchmarks don't care the test apps are not actually buildable, so maybe we can remove the shim references anyway... I guess I will leave that to tooling though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds like more detangling is needed.
@@ -21,8 +21,6 @@ | |||
<PackageReference Include="Microsoft.CodeAnalysis.Workspaces.Common" VersionOverride="$(MicrosoftCodeAnalysisWorkspacesCommonPackageVersion)" /> | |||
<ProjectReference Include="..\..\Microsoft.CodeAnalysis.Razor.Compiler\src\Microsoft.CodeAnalysis.Razor.Compiler.csproj" /> | |||
<ProjectReference Include="..\..\..\Shared\Microsoft.AspNetCore.Razor.Test.Common\Microsoft.AspNetCore.Razor.Test.Common.csproj" /> | |||
<!-- Included for definitions of Tag Helper types --> | |||
<ProjectReference Include="..\..\test\Microsoft.AspNetCore.Razor.Test.MvcShim\Microsoft.AspNetCore.Razor.Test.MvcShim.Compiler.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 FWIW, it looks like the last remaining reference to MvcShim.Compiler.csproj is in Microsofot.AspNetCore.Mvc.Razor.Extensions.Test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, MVC extensions still use it, that's next on my todo list
@dotnet/razor-compiler for a second review, thanks (technically this is just tests, but I'm not sure if it qualifies for one sign off only) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jan, really appreciate how much this cleans up. Only a couple of minor formatting comments, otherwise LGTM.
.Metadata(PropertyName(nameof(NestedEnumTagHelper.NestedEnumProperty))) | ||
.TypeName($"{typeof(NestedEnumTagHelper).FullName}.{nameof(NestedEnumTagHelper.NestedEnum)}") | ||
.Metadata(PropertyName("NestedEnumProperty")) | ||
.TypeName($"{"TestNamespace.NestedEnumTagHelper"}.{"NestedEnum"}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.TypeName($"{"TestNamespace.NestedEnumTagHelper"}.{"NestedEnum"}") | |
.TypeName("TestNamespace.NestedEnumTagHelper.NestedEnum") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks. I used some fancy regex to refactor this file, so no wonder this happened.
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that it really conveyed my suggestion in this diff, just saying to remove the trailing newline.
Part of #10343.