Action message select overloaded function #314 #739
Conversation
| let methodParameters = method.GetParameters() | ||
| where message.Parameters.Count == methodParameters.Length | ||
| select method).FirstOrDefault(); | ||
| //return (from method in target.GetType().GetRuntimeMethods() |
There was a problem hiding this comment.
could we remove the commented out code? If we need to see old code it should be in the history for the repo
| #else | ||
| //return | ||
| //return (from method in target.GetType().GetMethods() |
There was a problem hiding this comment.
Could we remove commented out code?
| let methodParameters = method.GetParameters() | ||
| where message.Parameters.Count == methodParameters.Length | ||
| select method).FirstOrDefault(); | ||
| //return (from method in target.GetType().GetRuntimeMethods() |
There was a problem hiding this comment.
Could we remove the commented out code?
|
@KasperSK the code looks good to me. When the code gets merged to master could you add a blog post about this? |
|
@vb2ae Sure a blog post would make sense, and I would love to write it. I had a thought what if we made the strict parsing optional? That way we could still get the old behavior where strings are converted to numbers. |
|
@KasperSK true a flag to turn it on or off would be a good idea. Do you think strict parsing should be on or off by default? |
|
Yeah, I suggest making this an opt in feature as it potentially could cause a breaking change in what action gets called. |
|
@vb2ae I think current behavior should be the default. I was thinking about making a global flag to opt in for all action messages and a local flag to overwrite behavior for a single action message. This will enable the old functionality when needed otherwise I think we need to look into having the Parser assigning type when parsing and I don't think that would be a good idea. @nigel-sampson Do you think it would be best to put the global flag on the ActionMessage class as a static property? |
|
I'd suggest making it a global fun with some relevant parameters, this way the behavior could be dynamic based on the inputs. If you're making this on by default then I'd suggest adding some good docs around the change in behavior for the migration documentation. |
|
@KasperSK keeping the same behavior by default is probably the best way to do it |
|
@KasperSK should we Merge this in? |
|
I believe that I still need to make it so that you can change between the old and the new behavior. Might look at it this week. Then we should be able to merge this in. |
This is a proposal on how to fix #314, the implementation in this pull request is more strict than Caliburn.Micro has been in the past.
If there is no method found that matches the parameter types supplied it will return null. This means that a string wont be converted to int even if it is possible. It will take into account derived classes.
I would love some feedback on this @vb2ae, @nigel-sampson.