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

Open
wants to merge 5 commits into
base: master
from
Open

Conversation

@AlexCatarino
Copy link
Member

@AlexCatarino AlexCatarino commented Dec 19, 2019

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

s4v4g3 and others added 3 commits Sep 12, 2019
* 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.
@AlexCatarino AlexCatarino requested a review from Martin-Molinero Dec 19, 2019
@codecov-io
Copy link

@codecov-io 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
@jaredbroad jaredbroad requested a review from Martin-Molinero Jun 14, 2020
Copy link
Member

@Martin-Molinero Martin-Molinero left a comment

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];

This comment has been minimized.

@Martin-Molinero

Martin-Molinero Jun 15, 2020
Member

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

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.