Skip to content

Temporary fix method binder for out parameters #1672

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

Merged
merged 7 commits into from
Jan 14, 2022

Conversation

eirannejad
Copy link
Contributor

@eirannejad eirannejad commented Jan 13, 2022

What does this implement/fix? Explain your changes.

Current method binder code needs cleanup. There is an active issue with pythonnet failing to find the appropriate overload when the method signature includes out parameters e.g. SomeMethod(int a, out int b). Current workaround is to call the method by providing bogus value for the out parameter e.g. SomeMethod(1, 0) instead of SomeMethod(1)

This PR pushes a temporary fix for this problem until the binder code is cleaned up. The change is intentionally minimal.

Does this close any currently open issues?

Not sure. Can not find anything related to this by looking at open issue titles

Any other comments?

No

Checklist

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@eirannejad eirannejad changed the title temp fixed method binder for out params Temporary fix method binder for out parameters Jan 13, 2022
@lostmsu
Copy link
Member

lostmsu commented Jan 13, 2022

@eirannejad, please add a test or two to cover the new code.

existing tests are passing bogus values to force matching the method signature
these tests should pass with the recent method binder fix, without passing the bogus values
Copy link
Member

@lostmsu lostmsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, keep old test scenarios unless they are actually not supported anymore.

@eirannejad eirannejad requested a review from lostmsu January 14, 2022 02:00
@eirannejad
Copy link
Contributor Author

(I'm a bit new to Github review/request change process. I apologize in advance if I clicked too many things)

@lostmsu lostmsu merged commit f69753c into pythonnet:master Jan 14, 2022
@filmor
Copy link
Member

filmor commented Jan 14, 2022

@eirannejad We'd need you to sign the .NET Foundation CLA: https://cla.dotnetfoundation.org/pythonnet/pythonnet

@lostmsu Please have an eye on this for first-time contributors until the CLA bot is fixed.

@eirannejad eirannejad deleted the pr/outparamfix branch January 14, 2022 17:17
@eirannejad
Copy link
Contributor Author

@filmor @lostmsu Signed the CLA. Thanks for merging this.

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.

3 participants