-
-
Notifications
You must be signed in to change notification settings - Fork 26
Enable calling Java methods that take arrays #53
Conversation
9c20c5d
to
107f4e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic mechanics look sound and well tested. The edge cases are my only concern - both in terms of the ways the API can be used, and APIs that people might want to use. Details inline.
arg_types.append([b'[I']) | ||
elif isinstance(arg[0], (float, jfloat, jdouble)): | ||
if all((isinstance(item, (float, jfloat, jdouble)) for item in arg)): | ||
arg_types.append([b'[D', b'[F']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the fallback case here? If arg
is a sequence of non-zero length, and types aren't consistent, this won't throw an unknown argument error (or any other type of error) - it will move on to trying to create an option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also seem to rule out passing in an empty array under any circumstances...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both should be clarified in a fresh push
rubicon/java/api.py
Outdated
elif type_name[1] == ord(b'D'): | ||
jarg = java.NewDoubleArray(len(arg)) | ||
java.SetDoubleArrayRegion(jarg, 0, len(arg), (jdouble * len(arg))(*arg)) | ||
converted.append(jarg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to not include byte, char, bool, and any other primitive for which there is a SetXArrayRegion method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bytes are already smoothly handled by the b'xyz'
handling a few lines before this.
char
and bool
... sure... but I just haven't really seen 'em in the wild, so I'm just being lazy.
I can try adding them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For char
arrays, I want to punt. For the rest, they're now handled.
rubicon/java/types.py
Outdated
@@ -19,7 +19,7 @@ | |||
jbyte = c_byte | |||
jchar = c_wchar | |||
jshort = c_short | |||
jint = c_int | |||
jint = c_uint32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? Aren't Java ints 32 bit signed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right-o -- I meant to specify c_int32. Fixed now.
I am a little weirded-out to say c_int
rather than c_int32
, so I like the latter, but I'm OK if we don't make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there - one more edge case I can see.
if all((isinstance(item, (float, jfloat, jdouble)) for item in arg)): | ||
arg_types.append([b'[D', b'[F']) | ||
else: | ||
raise ValueError("Unable to treat all data in list as floats/doubles") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a final else clause here rounding out an arg that is a sequence of nonzero length, but where the first element isn't a bool, int, float or double.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
with self.assertRaises(ValueError): | ||
Example.sum_all_floats([1.0, "two"]) | ||
with self.assertRaises(ValueError): | ||
Example.sum_all_doubles([1.0, "two"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test case here is 'first argument == "two"'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
PR Checklist: