-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Add RazorLangVersion 5.0 #23408
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
Add RazorLangVersion 5.0 #23408
Conversation
| var result = await DotnetMSBuild( | ||
| "Build", | ||
| "/p:UseRazorBuildServer=false /p:RazorLangVersion=5.0", | ||
| "/p:UseRazorBuildServer=false /p:RazorLangVersion=6.0", |
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.
Minor point, but why keep rolling this forward each time? What about:
| "/p:UseRazorBuildServer=false /p:RazorLangVersion=6.0", | |
| "/p:UseRazorBuildServer=false /p:RazorLangVersion=99999.0", |
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.
I don't really know why this test is authored this way. @NTaylorMullen \ @ajaybhargavb ?
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.
No reason. I was probably being dumb.
SteveSandersonMS
left a comment
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 a lot for sorting this out, @pranavkm!
| Assert.BuildOutputContainsLine( | ||
| result, | ||
| $"Invalid option 5.0 for Razor language version --version; must be Latest or a valid version in range 1.0 to 3.0."); | ||
| $"Invalid option 99.0 for Razor language version --version; must be Latest or a valid version in range 1.0 to 5.0."); |
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.
or a valid version in range 1.0 to 5.0.
This is a confusing statement indicating 4.0 might be valid. Could we rephrase this somehow?
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.
Suggestions? I would note that none of the point values are supported. (e.g. 3.1 is not valid).
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.
If it is easy to do, we could just list all the possible supported values. Just like how dotnet lists all the available SDKs
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.
Here's what the compiler does:
Error CS1617 Invalid option '9.0' for /langversion. Use '/langversion:?' to list supported values
It's not really easy to find this. I'm kinda tempted to leave these be. AFAIK, nobody configures these. Plus the language versions are precariously tied to the runtime version that setting this is always going to get you in trouble.
| version = Experimental; | ||
| return true; | ||
| } | ||
| else if (languageVersion == "5.0") |
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.
You need to add this equivalent to the tooling repo too: https://github.com/dotnet/aspnetcore-tooling/blob/671d0792fb3fb587766a5800fafb0eee2d8f43cd/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/FallbackRazorConfiguration.cs#L37-L39
I spoke to Taylor offline and it looks like we can get away without creating a new RazorConfiguration or extension for MVC. I did some testing with MVC 5.0 and 3.1 apps, and tooling appears to work correctly.