X Tutup
The Wayback Machine - https://web.archive.org/web/20220413052502/https://github.com/pythonnet/pythonnet/pull/1191
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

Jump to bottom

match python type T to Nullable<T> in C# #1191

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

match python type T to Nullable<T> in C# #1191

wants to merge 2 commits into from

Conversation

hatami57
Copy link

@hatami57 hatami57 commented Jul 30, 2020 •
edited

When a method parameter in C# is defined as Nullable like int? and you call it in python by like a literal int number, it does not match with int? and could not bind the method, at least in .NETStandard2.0. So this update is telling the MethodBinder class that a primitive value (with the type of T) in python can be matched with the corresponding primitive type (T) in C# or the Nullable<T> type too.

What does this implement/fix? Explain your changes.

...

Does this close any currently open issues?

...

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

@hatami57
match python type T to Nullable<T> in C#
9135b94 
When a method parameter in C# is defined as Nullable like `int?` and you call it in python by a literal int number, it does not match with `int?` and could not bind the method. At least in .NETStandard2.0. So this update is telling the MethodBinder class that a primitive value (with the type of T) in python can be matched with the corresponding primitive type (T) in C# or Nullable<T> too.
@dnfadmin
Copy link

@dnfadmin dnfadmin commented Jul 30, 2020 •
edited

CLA assistant check
All CLA requirements met.

@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Jul 30, 2020 •
edited

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1191   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          251      251           
  Misses         40       40
Flag Coverage Δ
#setup_linux 64.94% <ø> (ø)
#setup_windows 72.50% <ø> (ø)

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 7a9dcfa...9135b94. Read the comment docs.

@lostmsu
Copy link
Member

@lostmsu lostmsu commented Jul 30, 2020 •
edited

Honestly, I am not even sure we need TryComputeArgumentType. The only thing it provides right now is resolving between Foo(int) and Foo(double), which should be handled in a different way altogether: the current version looks like it won't let you call Foo(double, string) by doing Foo(1, "42") in Python.

@hatami57
Copy link
Author

@hatami57 hatami57 commented Jul 30, 2020 •
edited

Anyway I couldn't call a C# class constructor say Foo(int, int, double?) by calling Foo(2, 5, 0.25) in Python. So with this minor change in the code, it just works.

For now, I think TryComputeArgumentType do an important job in the library and it needs some improvements. If the logic needs refactoring, still this minor change need to be considered...

@lostmsu
Copy link
Member

@lostmsu lostmsu commented Jul 30, 2020

@hatami57 can you try passing a Nullable(0.25) or Nullable[Double](0.25) as a workaround?

@hatami57
Copy link
Author

@hatami57 hatami57 commented Aug 4, 2020

@hatami57 can you try passing a Nullable(0.25) or Nullable[Double](0.25) as a workaround?

As far as I remember, I did try it but it didn't work...

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 16, 2020

Codecov Report

Merging #1191 (9135b94) into master (f8c27a1) will increase coverage by 12.20%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1191       +/-   ##
===========================================
+ Coverage   74.04%   86.25%   +12.20%     
===========================================
  Files           1        1               
  Lines         289      291        +2     
===========================================
+ Hits          214      251       +37     
+ Misses         75       40       -35
Flag Coverage Δ
setup_linux 64.94% <ø> (?)
setup_windows 72.50% <ø> (-1.54%) ⬇️

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

Impacted Files Coverage Δ
setup.py 86.25% <0.00%> (+12.20%) ⬆️

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 4133927...e3bf747. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@hatami57 @dnfadmin @codecov-commenter @lostmsu @codecov-io @filmor
X Tutup