search

Caliburn-Micro / Caliburn.Micro

A small, yet powerful framework, designed for building applications across all XAML platforms. Its strong support for MV* patterns will enable you to build your solution quickly, without the need to sacrifice code quality or testability.
http://caliburnmicro.com/
MIT License
2.8k stars 778 forks source link

Action message select overloaded function #314 #739

Open KasperSK opened 3 years ago

KasperSK commented 3 years ago

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.

vb2ae commented 3 years ago

@KasperSK the code looks good to me. When the code gets merged to master could you add a blog post about this?

KasperSK commented 3 years ago

@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.

vb2ae commented 3 years ago

@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?

nigel-sampson commented 3 years ago

Yeah, I suggest making this an opt in feature as it potentially could cause a breaking change in what action gets called.

KasperSK commented 3 years ago

@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?

nigel-sampson commented 3 years ago

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.

vb2ae commented 3 years ago

@KasperSK keeping the same behavior by default is probably the best way to do it

vb2ae commented 3 years ago

@KasperSK should we Merge this in?

KasperSK commented 3 years ago

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.