-
Notifications
You must be signed in to change notification settings - Fork 229
Clean up RazorConfiguration, RazorLanguageVersion, and RazorExtension types #10001
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
Clean up RazorConfiguration, RazorLanguageVersion, and RazorExtension types #10001
Conversation
Now that the compiler assemblies have been merged, we can go avoid using assembly-level attributes and reflection to initialize Razor extensions.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorLanguageVersion.cs
Outdated
Show resolved
Hide resolved
...azor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/FallbackRazorConfiguration.cs
Show resolved
Hide resolved
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.
This is used in the SDK:
Are you planning to update that as well?
(Note: it would be probably best to bring the whole RazorTool from sdk repo to this repo to avoid these kind of breaks.)
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 to me like that instantiation of AssemblyExtension is never even used. 😄
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.
Ah, no. it's likely used by this code, which I'm removing. I have no idea how to update the SDK with new packages, so I'm not really sure how to make the ultimate change. However, I'm cloning the repo and will have commits in a branch ready for when we flow into the SDK.
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.
And yes, we should definitely bring all of that over to this repo. 👍
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've created DustinCampbell/sdk@6425721 to fix up the source breaks in the Razor SDK tools. However, these tools look even more suspect right now, since they take arguments to specify extension file paths and names that will effectively be ignored. I'm not sure how or if those are used 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.
However, these tools look even more suspect right now, since they take arguments to specify extension file paths and names that will effectively be ignored. I'm not sure how or if those are used though.
They are not used anymore, there's a hardcoded mapping:
I've created DustinCampbell/sdk@6425721 to fix up the source breaks in the Razor SDK tools.
LGTM, thanks.
|
@jjonescz: Any other concerns? Just need a second sign off from the compiler team. |
jjonescz
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.
LGTM, thanks!
This change converts
RazorConfiguration,RazorLanguageVersion, andRazorExtensionto sealed records and removes all extraneous subtypes. To achieve this, I've also removed theAssemblyExtensiontype along withProvideRazorExtensionInitializerAttribute. After the compiler assembly merge, the set of Razor extensions is known. So, I've removed the reflection code that would initialize a Razor extension and just handle the extensions statically.