Skip to content

overloading - more fixes for wordpad.py #170

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.

Sign up for GitHub

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
Closed
wants to merge 3 commits into from
Closed

overloading - more fixes for wordpad.py #170

wants to merge 3 commits into from

Conversation

den-run-ai
Copy link
Contributor

System.Drawing.Font("Tahoma", 10.0) supposed to use Single, which does not directly map to Python. Hence I added comparison in both directions for Double/Single, Int32/Int16.

This fixes issue observed by @mahibra that I reproduced today:

#103 (comment)

tonyroberts
tonyroberts reviewed Feb 27, 2016
View reviewed changes
src/runtime/methodbinder.cs
if (clrtype != null) {
if (pi[n].ParameterType != clrtype) {
IntPtr pytype = Converter.GetPythonTypeByAlias(pi[n].ParameterType);
// does pytype require Decref?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a question? it's your code, you should understand it. Comments should help the people that come after you to understand the code, not raise questions about its correctness.

@tonyroberts
Copy link
Contributor

See contributing.md about grouping commits into logical units (which you've been told about numerous times already).

I think this change warrants a new unit test to demonstrate the exact problem. In the meantime, if a user is having a problem a simply workaround is to construct the exact C# type an call the method with that (not that I'm against having this fixed in the main code).

@mahibra
Copy link

mahibra commented Feb 29, 2016

@denfromufa: With the modified version I am able to run the wordpad.py example. However, the commercial API still crashes when calling the method ReadNextResult with the following arguments:

batchRayTrace.ReadNextResult((0, 0, 0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0))
Traceback (most recent call last):
  File "C:\Anaconda3\lib\site-packages\IPython\core\interactiveshell.py", line 3066, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-6-41428a5b8fa5>", line 1, in <module>
    batchRayTrace.ReadNextResult((0, 0, 0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0))
TypeError: No method matches given arguments

with

batchRayTrace.ReadNextResult?
Type:        MethodBinding
String form: <bound method 'ReadNextResult'>
File:        c:\anaconda3\lib\site-packages\clr.pyd
Docstring:
Boolean ReadNextResult(Int32 ByRef, Int32 ByRef, Int32 ByRef, Int32 ByRef)
Boolean ReadNextResult(Int32 ByRef, Int32 ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef)
Boolean ReadNextResult(Int32 ByRef, Int32 ByRef, Int32 ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef)
Boolean ReadNextResult(Int32 ByRef, Int32 ByRef, Int32 ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef)
Boolean ReadNextResult(Int32 ByRef, Int32 ByRef, Int32 ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef, Double ByRef)

However, it works with the methodbinder.cs version prior to patch #151

@den-run-ai
Copy link
Contributor Author

@mahibra I found out the problem last night while writing tests for improved overloading resolution in pythonnet, but have not finished my improved patch yet. Note that before it was working by accident, i.e. no type checking was done which could give you wrong results like this:

#131

The problem stems from Python 2 to 3 change of int to always long implementation and no equivalent in .NET among Int16, Int32, Int64. Right now Int64 maps to Python long, but your overloaded methods require Int32. So try explicit types such as Int32(0) and let us know if it works that way.

@mahibra
Copy link

mahibra commented Feb 29, 2016

The following command works:

from System import Int32, Double
batchRayTrace.ReadNextResult(Int32(0), Int32(0), Int32(0), Double(0.0), Double(0.0), Double(0.0), Double(0.0), Double(0.0), Double(0.0), Double(0.0), Double(0.0), Double(0.0), Double(0.0), Double(0.0))

I have to convert both the Int and the Double values. Otherwise I get the error
TypeError: No method matches given arguments

@den-run-ai den-run-ai mentioned this pull request Mar 3, 2016
Closed
@den-run-ai
Copy link
Contributor Author

closing this in favor of #175

@den-run-ai den-run-ai closed this Mar 3, 2016
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.
4 participants
@den-run-ai @tonyroberts @mahibra @vmuriart