Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi parameter delegation #47

Closed
wants to merge 4 commits into from
Closed

Multi parameter delegation #47

wants to merge 4 commits into from

Conversation

hzy-6
Copy link

@hzy-6 hzy-6 commented Jan 27, 2024

#3 (comment)

sum(1,2,3,4 ... )

@VitaliyMF
Copy link
Contributor

Sorry, I cannot accept this PR:

  • it doesn't add "params" support for delegate calls in fact
  • changes in LambdaParameterWrapper are contradictory. It is wrong assumption that if delegate has 1 argument with object type we can pass all arguments as an array - this is not support of "params" at all
  • support of params in LambdaParameterWrapper.InvokeDelegate should be implemented in the same way as in LambdaParameterWrapper.InvokeMethod: invocation is delegated to IInvokeMethod implementation, which checks for ParamArrayAttribute of the last delegate argument and handles this accordingly.

@VitaliyMF VitaliyMF closed this Jan 27, 2024
@hzy-6
Copy link
Author

hzy-6 commented Jan 27, 2024

Is this okay?

image

image

@hzy-6
Copy link
Author

hzy-6 commented Jan 27, 2024

It may be better to leave the InvokeDelegate function to developers for implementation

@VitaliyMF
Copy link
Contributor

@hzy-6 an approach in overall is correct (first screenshot), however implementation in OptionsParamInvokeMethod is definitely wrong - take a look how correct handling of "params" is implemented in OptionsParamInvokeMethod.Invoke method; also, I guess some refactoring is needed to avoid copy paste between these 2 methods.

If you don't know how to implement this, you can use your own implementation which works for you for now (your forked version), and then switch to upstream when it will be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants