- Notifications
You must be signed in to change notification settings - Fork749
Ensure that param-array matching works correctly#1304
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedDec 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #1304 +/- ##======================================= Coverage 74.04% 74.04% ======================================= Files 1 1 Lines 289 289 ======================================= Hits 214 214 Misses 75 75
Flags with carried forward coverage won't be shown.Click here to find out more. Continue to review full report at Codecov.
|
Can you add a test with a mix of keyword and params array arguments? |
I added some more tests, but three of them are failing. I commented them out for now and will try to fix them today. |
Uh oh!
There was an error while loading.Please reload this page.
lostmsu left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please, remove the commented out code first.
We can match n Python parameters to exactly n+1 C# parameters of whichthe last one is a param-array.Fixespythonnet#1302.
| @@ -652,7 +652,7 @@ static Type TryComputeClrArgumentType(Type parameterType, IntPtr argument, bool | |||
| { | |||
| match = true; | |||
| } | |||
| else if (positionalArgumentCount < parameters.Length) | |||
| else if (positionalArgumentCount < parameters.Length && (!paramsArray || positionalArgumentCount == parameters.Length - 1)) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe it would be useful to comment explicitly on what kind of cases would fall into this condition, or simplify the branching statements somehow (refactor the default arg logic in a separate method)? Just by reading this I'm guessing that this should take care of only the following 2 cases?
- when it's not a
paramsarray but there are some default values we need to fill in - when it's a
paramsarray and exactly one parameter is missing
Maybe there are more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nevermind, actually I get it now! I also added an extra comment and renamed the variables for readability and consistency with the others parts of methodbinder in my PR:https://github.com/pythonnet/pythonnet/pull/1324/files#diff-237dc718ad5424f5476d2ebbbc4e5164c95901bd2a4c2daaf8ca6fc9f9d3a5fbR703-R704
Uh oh!
There was an error while loading.Please reload this page.
What does this implement/fix? Explain your changes.
The previous matching code wasn't specific enough for param arrays, the count of parameters has to be exactly right.
Does this close any currently open issues?
#1302
Any other comments?
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG