X Tutup
Skip to content

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

Closed
hatami57 wants to merge 2 commits intopythonnet:masterfrom
hatami57:master
Closed

match python type T to Nullable<T> in C##1191
hatami57 wants to merge 2 commits intopythonnet:masterfrom
hatami57:master

Conversation

@hatami57
Copy link
Copy Markdown

@hatami57 hatami57 commented Jul 30, 2020 •
edited
Loading

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
Copy Markdown

dnfadmin commented Jul 30, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 30, 2020 •
edited
Loading

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
Copy Markdown
Member

lostmsu commented Jul 30, 2020 •
edited
Loading

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
Copy Markdown
Author

hatami57 commented Jul 30, 2020 •
edited
Loading

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
Copy Markdown
Member

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
Copy Markdown
Author

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
Copy Markdown

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.

@filmor
Copy link
Copy Markdown
Member

filmor commented Oct 28, 2022

@hatami57 Did you try what lostmsu suggested again on 3.0?

@filmor filmor closed this Nov 10, 2022
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.

6 participants

@hatami57 @dnfadmin @codecov-commenter @lostmsu @codecov-io @filmor
X Tutup