Make sure migration model has type mapping #20652
Conversation
AndriySvyryd
force-pushed
the
Issue20517
branch
from
d018092
to
6208f18
Compare
|
|
||
| private IModel FinalizeModel(IModel model) | ||
| { | ||
| if (model is IConventionModel conventionModel) |
There was a problem hiding this comment.
Should this be a Check() instead? When is it not an IConventionModel?
There was a problem hiding this comment.
Migration exposes it as IModel, so enforcing that it has to be an IConventionModel goes against Liskov's principle. Even though in all practical scenarios in would be one.
There was a problem hiding this comment.
Check.DebugAssert() then? My point is that if the model is not an IConventionModel and you expect this code to do something, it will fail to do so silently, and that could be hard to track down later.
If there are valid scenarios where this would not be an IConventionModel and this code should do nothing, then you can ignore this comment. I just wasn't sure what those would be.
There was a problem hiding this comment.
If it's not an IConventionModel we can't do anything to it, so we'll just assume that it already has the expected annotations, if it doesn't it will be obvious from the exception messages thrown after this.
|
Better late than never 😆 |
|
lol, yep. Slowly catching up... |
Fixes #20517