Extensions refactor #10850
Extensions refactor #10850
Conversation
- Update the MVC3 version to optionally not enforce nullability to keep code gen identical
|
@dotnet/roslyn-compiler for review please :) |
chsienki
force-pushed
the
extensions_refactor
branch
from
7e6a83a
to
96a979d
Compare
| var irDocument = codeDocument.GetDocumentIntermediateNode(); | ||
| irDocument.DocumentKind = MvcViewDocumentClassifierPass.MvcViewDocumentKind; | ||
| return irDocument; | ||
| return codeDocument.GetDocumentIntermediateNode(); |
There was a problem hiding this comment.
Is this no longer needed?
There was a problem hiding this comment.
The tests weren't registering the MvcViewDocumentClassifierPass so the document type wasn't set. I changed the tests to include that pass so there's no need to manually override it anymore.
There was a problem hiding this comment.
Thanks for the information!
Sigh... This sort of test infrastructure is odd to me. Why not just run the tests on the real thing instead of hacking and setting random properties to make it seem as if it's the real thing? I saw something very similar in tests when removing the CodeRenderingContext.Items property.
|
@dotnet/razor-compiler for a second review please :) |
| @@ -42,7 +42,7 @@ public void WriteInjectProperty(CodeRenderingContext context, InjectIntermediate | |||
| else if (!node.IsMalformed) | |||
| { | |||
| var property = $"public {node.TypeName} {node.MemberName} {{ get; private set; }}"; | |||
| if (!context.Options.SuppressNullabilityEnforcement) | |||
| if (considerNullabilityEnforcement && !context.Options.SuppressNullabilityEnforcement) | |||
There was a problem hiding this comment.
Can't we set context.Options.SuppressNullabilityEnforcement in the tests where we need it instead of introducing a new test-only parameter?
There was a problem hiding this comment.
No, this isn't about testing, but actual implementation. We don't ever want to write out the nullability info for the older MVC versions. There probably isn't any harm in doing so, but I wanted to make sure the emitted code was exactly the same as part of this PR.
See here for where they are used in non-test code: 712bd0d#diff-156fa1883266f94c39e6b7474a75a11fc3c382ff439d8fedd22727f5a5b0c6faR23
There was a problem hiding this comment.
So we removed some Mvc.Version1_X and Mvc.Version2_X classes. Does the compiler now pick up Mvc (v3) equivalents instead? How does that work?
There was a problem hiding this comment.
Yes. They have the same names, and the previous V1/V2 classes were in 'deeper' namespaces. With those gone, the references now resolve to the 'shallower' namespace'd V3 versions.
Deletes code that was duplicated across the MVC3/2/1 extensions.
I made changes in fairly small grouped commits, so commit-by-commit might be easier. Almost all of them are just deletes though, with the exception of 712bd0d which added a parameter to control the output for V1&2
At each commit I ran tests and verified everything was green. There is more code we can de-duplicate but the rest requires more involved parameters to control so I figured I would keep the diffs here dirt simple.