Added support for optional parameters in method calls

[microspud]

Added support for optional parameters in method calls Handles a possible null reference exception in InvokePropertyOrField Handles a possible AmbiguousMatchException in InvokePropertyOrField with shadowed properties

[VitaliyMF]

Finally I find a time to review these changes:

• LambdaParserTests.cs - it is unclear why "using System.Transactions;" is added, why it is needed for the test?
• formatting: maybe this is just an automatic reformat made by your VS according to your formatting settings. In LambdaParser code setting are: keep { on the same line and indent = tabs.
• I propose to add an option to enable optional parameters, similar to existing settings in LambdaParser (like AllowVars). From what I see, the criteria how overloaded method is selected is changed, and I may imagine a case when new code will choose another overload - I understand that this is a very unlikely on practice - but I would like to guarantee 100% backward compatibility. By default optional parameters should not be supported, and criteria how overloaded method is chosen should remain as in original code. To do that in an elegant way I guess some refactoring should be performed, InvokeMethod is currently created inside LambdaParameterWrapper, it may become a singleton created in LambdaParser ( its constructor parameters may be easily moved to Invoke arguments) and may have an option like 'EnableOptionalMethodParams'.

I really appreciate your contribution. Please let me know if you don't know how to implement the last item, in this case I can do that by myself after accepting your PR.

[microspud]

Hi, Sorry for the delay in replying. I have been away on business this week and only got back home last night. Thank you for your feedback, I will implement as you suggest above:

• EnableOptionalMethodParams
• Refactor InvokeMethod.cs, I see no reason why it can't be a singleton. It will be more memory efficient as well.
• Remove System.Transactions since it was automatically included by visual studio randomly.
• Fix formatting

Also note that I subsequently created another pull request including params support #45. I decided to include the changes contained in this pull request as well because it would have resulted in merge issues if I had. I propose we close this one and work on #45 instead if you are happy to do that?

No problems contributing, I have enjoyed the challenge and I really appreciate you writing the framework in the first place as it has been very useful to me. :)

[VitaliyMF]

I propose we close this one and work on https://github.com/nreco/lambdaparser/pull/45 instead if you are happy to do that?

Sure, these changes are closely related, it makes sense to have a one PR for them.

[microspud]

Hi Vitaliy, I have finally had some time to work on the improvements. Basically its done, but regarding InvokeMethod singleton within LambdaParser, I have coded this, but as far as I can tell, we will need to pass the reference to the constructor of LambdaParameterWrapper globally. This seems a little over overkill. I was wondering if perhaps we can make InvokeMethod static and avoid this? What is your opinion? Many Thanks, Richard

[VitaliyMF]

Basically its done, but regarding InvokeMethod singleton within LambdaParser, I have coded this, but as far as I can tell, we will need to pass the reference to the constructor of LambdaParameterWrapper globally. This seems a little over overkill.

LambdaParameterWrapper constructor already depends on IValueComparer (which comes from LambdaParser instance) so this should not be a problem at all, to reference also concrete instance of InvokeMethod. Static class is definitely the worst possible variant (don't use statics in libraries, never ever :) ).

BTW, and additional abstraction may be introduced here via new IInvokeMethod interface - so implementation of InvokeMethod may be re-defined in applications, when needed. This brings me to idea of having 2 implementations - one is an old one (that currently in 'master') and another one smth like OptionsParamsInvokeMethod class with enhancements you made. And this allows to have own app-specific implementation if someone will need it.

Finally, LambdaParser will have InvokeMethod property initialized with DefaultInvokeMethod and only when needed, it may be replaced with OptionsParamsInvokeMethod -- what do you think about this?

I really worry about backward compatibility, because I personally use NReco.LamdbaParser in my own product (SeekTable) where expressions are entered by end users and I simply have no technical possibility to ensure that all these expressions remain working as before (more than that, I don't have these expressions because SeekTable is on-premises product).

No problems if you don't have a time for that, just let me know and I'll accept your PR and re-organize the code as needed. I hope there will be some time for that on Christmas holidays ^_^

[microspud]

Hi,

Ok about IValueComparer, I didn't notice that, will use that.

I think using an interface for InvokeMethod is a great idea! We can use the currently released version as it is for backwards compatibility and offer the new functionality in alternative descendant classes.

I propose that I can make this change and use the existing InvokeMethod class, and create a copy for the new functionality. We could abstract it correctly later into abstract class and the like.

I will have some time in the next couple of days. I hope to get it done by 25th so that you can take a look over the festive period :)

[microspud]

Hi Vitaliy,

Christmas got in the way. I have now made the modifications. I hope that it is what you were expecting. Please let me know if there is anything that needs improvement. As far as I can tell I have not changed anything that should affect the default operation of the component externally. I used overloaded constructure rather than optional parameters for example in LamdaParser class. Also InvokeMethod is still internal, however the new OptionsParamsInvokeMethod class needs to be public so that users can pass it to the constructor of LamdaParser.

Anyhow let me know your thoughts when you get time.

BTW I noticed there was one static reference to IsInstanceOfType in InvokeMethod used by InvokeDelegate... I have left this alone for now. Also I followed the design Pattern used by IValueComparer with its static internal instance. I'm not sure if this is a good idea based on what you said before however its easy to change if you think we should abandon that way.

Happy New Year.

Regards,

Richard

[VitaliyMF]

@microspud Looks good!

Also InvokeMethod is still internal

In fact we can make it public. And maybe refactor it a bit, and then derive OptionsParamsInvokeMethod from InvokeMethod to reuse similar parts?.. Anyway this can be made in a separate PR later. If you want to do that feel free to create a new issue.

I don't want to bother you with minor things like formattings / naming conventions, it much easier & faster to change that by myself. After that I'll prepare a new release.

Thank you a lot for all your contribution and Happy New Year! For me Peaceful New Year would be even better ^_^