MRCollective / ChameleonForms

Shape-shifting your forms experience in ASP.NET Core MVC
MIT License
254 stars 56 forks source link

PartialModelExpression: return LambdaExpression instead of object #153

Closed fsateler closed 5 years ago

fsateler commented 7 years ago

While we cannot know the exact type parameters, we can know that it should be a lambda expression


I'm not sure about .Net/IL ABI norms, but I think this will require recompilation of calling code, as IL includes the return type in the method signature.

robdmoore commented 7 years ago

While this is more correct is there any need you have for this?

fsateler commented 7 years ago

Well, strictly speaking I don't need it. But I currently need to workaround #152 , and thus need to access the PartialModelExpression, and having it returned as object makes it a little more inconvenient:

var expr = ExpressionHelper.GetExpressionText((LambdaExpression)this.PartialModelExpression());

This is very minor and has a large impact. I would agree if you would delay this until other such invasive changes are done. I found this stackoverflow question that confirms this is an ABI break:

Changing a method signature

Kind: Binary-level Break

Languages affected: C# (VB and F# most likely, but untested)

API before change

public static class Foo
{
     public static void bar(int i);
}

API after change

public static class Foo
{
    public static bool bar(int i);
}

Sample client code working before change

Foo.bar(13);
robdmoore commented 7 years ago

I don't really care about the binary breaking change, but can you change the PR to refactor the usage to remove the cast too?

fsateler commented 7 years ago

@robdmoore the casts are not removable, because we still need a Expression<Func<TModel, TPartialModel>>. But at least we can accept a LambdaExpression.