-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Update Mvc to use LoggerMessageAttribute #38658
Conversation
@david-acker are you manually updating these logger messages? I used a code-fix to do this in the past because it's far too easy to introduce errors / typos with such a large number of messages and it's incredibly difficult to review these in a PR. https://github.com/pranavkm/LoggerConvert has a version of the codefix, perhaps you want to give that a go (in a separate branch so you don't lose these changes)? |
@pranavkm Ah, a code fix definitely seems like the best way to handle this. I just wrote a script to generate the logger message attribute and partial methods from the original I'll go back and use the code fix you created just to ensure that no errors or typos will slip through. |
Thanks for your PR. |
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.
@david-acker these changes look good. If you'd like to make this PR non-draft, I'd be happy to get it merged.
Sounds good! There's still a decent amount of the original |
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! We can always tackle the remaining messages iteratively.
@pranavkm this looks like it broke the build: https://dev.azure.com/dnceng/internal/_build/results?buildId=1562862&view=logs&j=1b89928a-2219-5ef9-602f-f95beb3da4dc&t=31281680-1367-5e42-8b2d-0ecd34ac2092 Can you fix (or revert if its not straightforward)? |
On it. |
@pranavkm Just put up a PR to fix this. Odd that the previous CI runs didn't catch this. |
Hi @david-acker. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
Updates the Mvc project to use the LoggerMessageAttribute for logging generation. ## Description The following files contained duplicate log event ids which had to be renumbered: - MvcCoreLoggerExtensions - MvcRazorLoggerExtensions - MvcViewFeaturesLoggerExtensions - RazorRuntimeCompilationLoggerExtensions Also fixes a typo in a method name in RazorRuntimeCompilationLoggerExtensions: ViewCompilerInvalidingCompiledFile --> ViewCompilerInvalidatingCompiledFile Contributes to dotnet#32087
Updates the Mvc project to use the LoggerMessageAttribute for logging generation.
Description
The following files contained duplicate log event ids which had to be renumbered:
Also fixes a typo in a method name in RazorRuntimeCompilationLoggerExtensions:
ViewCompilerInvalidingCompiledFile --> ViewCompilerInvalidatingCompiledFile
Completes the Mvc task for #32087