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

Adds Support to Named Arguments #41

Closed
wants to merge 5 commits into from

Conversation

AlexCatarino
Copy link
Member

Pick-cherry (pythonnet#849)) from pythonnet master and resolve conflicts.

s4v4g3 and others added 3 commits December 18, 2019 16:47
* Add support for named arguments (pythonnet#849)
* Remove kwarg check since it breaks the python-derived CLR class use-case
* Add named parameter test cases
* Update changelog and authors
* Add default params tests
Pick-cherry from pythonnet master and resolve conflicts.
@codecov-io
Copy link

codecov-io commented Dec 19, 2019

Codecov Report

Merging #41 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #41   +/-   ##
=======================================
  Coverage   69.42%   69.42%           
=======================================
  Files           1        1           
  Lines         278      278           
=======================================
  Hits          193      193           
  Misses         85       85
Flag Coverage Δ
#setup_linux 69.42% <ø> (ø) ⬆️
Impacted Files Coverage Δ
setup.py 69.42% <ø> (ø) ⬆️

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 1391bfc...04e6927. Read the comment docs.

Remoes `break` at `AssemblyManager.Initialize` added for debugging/testing.
src/runtime/methodbinder.cs Outdated Show resolved Hide resolved
src/runtime/methodbinder.cs Outdated Show resolved Hide resolved
src/runtime/methodbinder.cs Show resolved Hide resolved
src/runtime/methodbinder.cs Outdated Show resolved Hide resolved
Copy link
Member

@Martin-Molinero Martin-Molinero left a comment

Choose a reason for hiding this comment

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

Looks good 👍 leaving a minor performance suggestion, believe we need a new version bump. Would be nice to run some of Lean python performance benchmarks

IntPtr op;
if (hasNamedParam)
{
op = kwargDict[parameter.Name];
Copy link
Member

Choose a reason for hiding this comment

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

Think could use kwargDict.TryGetValue at the top and avoid accessing the dictionary twice

@Martin-Molinero
Copy link
Member

Closing in favor of #47

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.

4 participants