-
Notifications
You must be signed in to change notification settings - Fork 225
Conversation
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.
Should also add an integration scenario for the Mvc.Razor.Extensions where a user uses @inherits Foo<TModel>
in a _ViewImports.cshtml
and @model
in the main page. I was concerned in the original bug about this being tested as well.
|
||
namespace RazorCodeGen | ||
{ | ||
public class MyBasePage<TModel> : RazorPage<TModel> |
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.
The tests don't need to be runnable/compile. I'd nuke this class for the sake of simplicity.
Ah, I didn't know we already had the infrastructure in place to give our codegen tests Import files. Neat. 🆙📅 |
@@ -0,0 +1,2 @@ | |||
@using RazorCodeGen | |||
@inherits MyBasePage<string> base |
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.
Generally is a View you'd inherit from MyBasePage<TModel>
because we expect there to be a ViewDataDictionary<TModel>
property defined on the class.
I think we need a case where you inherit from MyBasePageForViews<TModel>
with a traditional MVC view and a case where you inherit from MyBasePageForPages
for an @page
.
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.
Do you just mean that they should both be TModel and one of them should have @page
? If not I'm not sure I follow since the models we're inheriting from don't exist.
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 isn't close enough to what users actually do, take a look at the comments.
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 seeing an import file with a @inherits MyPageModel<TModel>
and a child view with @model string
@@ -24,6 +23,35 @@ public class CodeGenerationIntegrationTest : IntegrationTestBase | |||
private static readonly RazorSourceDocument DefaultImports = MvcRazorTemplateEngine.GetDefaultImports(); | |||
|
|||
#region Runtime | |||
[Fact] | |||
public void Inherits_Runtime() |
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 specific test should be in the non-extensions codegen test project. @inherits
is a core Razor directive. Same with its DesignTime counterpart.
@@ -0,0 +1 @@ | |||
@model MyModel |
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.
What's this guy?
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 misread and did the opposite of what you asked for, will flip it.
72d39f3
to
21b406b
Compare
🆙📅 hopefully I understood the asks here. |
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.
Close
@@ -0,0 +1 @@ | |||
@inherits MyPageModel<TModel> foo |
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.
lol wht's this foo
at the end? It should totally trigger a parser error
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.
Also, Q: does this ViewImports impact other code generation tests or have we built in a convention to understand TestName_ImportsX
?
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 that's the case do we want me to fix it here too, or did I misinterpret that line?
As for your second question we have a convetion to understand TestName_ImportsX
, it's also used for the BasicImports
test in Microsoft.AspNetCore.Razor.Language.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.
If that's the case do we want me to fix it here too, or did I misinterpret that line?
Woa, ya I'd definitely fix that one too.
As for your second question we have a convetion to understand TestName_ImportsX, it's also used for the BasicImports test in Microsoft.AspNetCore.Razor.Language.Test.
Awesomeneses 👍
0799eb9
to
5b7b131
Compare
🆙📅 |
@rynowak ready for your review. |
5b7b131
to
8af9f65
Compare
Fixes #1142.