FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Ensure that param-array matching works correctly by filmor · Pull Request #1304 · pythonnet/pythonnet · GitHub

/ pythonnet Public

Ensure that param-array matching works correctly - #1304

Merged
filmor merged 3 commits into
pythonnet:masterfrom
filmor:fix-params-index
Dec 10, 2020
ConversationCommits (3)ChecksFiles changed
Merged

Ensure that param-array matching works correctly#1304
filmor merged 3 commits into
pythonnet:masterfrom
filmor:fix-params-index

Conversation

filmor commented Dec 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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.

  • 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

codecov-io commented Dec 3, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1304 (13871f2) into master (f8c27a1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1304   +/-   ##
=======================================
  Coverage   74.04%   74.04%           
=======================================
  Files           1        1           
  Lines         289      289           
=======================================
  Hits          214      214           
  Misses         75       75
Flag Coverage Δ
setup_windows 74.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8c27a1...13871f2. Read the comment docs.

lostmsu commented Dec 3, 2020

Copy link
Copy Markdown
Member

Can you add a test with a mix of keyword and params array arguments?

filmor commented Dec 4, 2020

Copy link
Copy Markdown
Member Author

I added some more tests, but three of them are failing. I commented them out for now and will try to fix them today.

lostmsu reviewed Dec 9, 2020
View reviewed changes
Comment thread src/tests/test_method.py
lostmsu approved these changes Dec 9, 2020
View reviewed changes

lostmsu left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please, remove the commented out code first.

Ensure that param-array matching works correctly
001aeec 
We can match n Python parameters to exactly n+1 C# parameters of which
the last one is a param-array.

Fixes pythonnet#1302.
Add tests for method matching in presence of param arrays
b9538b9
filmor mentioned this pull request Dec 10, 2020
Open
filmor merged commit 4133927 into pythonnet:master Dec 10, 2020
filmor deleted the fix-params-index branch December 10, 2020 17:47
christabella mentioned this pull request Dec 17, 2020
Merged
4 tasks
christabella reviewed Dec 17, 2020
View reviewed changes
Comment thread src/runtime/methodbinder.cs
match = true;
}
else if (positionalArgumentCount < parameters.Length)
else if (positionalArgumentCount < parameters.Length && (!paramsArray || positionalArgumentCount == parameters.Length - 1))

christabella Dec 17, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

  1. when it's not a params array but there are some default values we need to fill in
  2. when it's a params array and exactly one parameter is missing

Maybe there are more?

christabella Dec 17, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lostmsu lostmsu approved these changes
+1 more reviewer
christabella christabella left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL