Partial method enhancement #462
Conversation
tests/objc/Example.m
Outdated
|
|
||
| +(NSUInteger) overloaded:(NSUInteger)arg duplicateArg:(NSUInteger)arg1 duplicateArg:(NSUInteger)arg2 | ||
| { | ||
| return arg1 + arg2; |
There was a problem hiding this comment.
This won't confirm if arg1 and arg2 are being handled in the right order, as the addition is commutative. Something like 2*arg1 + 3*arg2 (or even arg1 - arg2) would let us confirm that arg1 and arg2 are being handled in the right order.
tests/test_core.py
Outdated
| Example = ObjCClass("Example") | ||
|
|
||
| self.assertEqual(Example.overloaded(0, extraArg1=0, extraArg2=0), 1) | ||
| self.assertEqual(Example.overloaded(0, extraArg2=0, extraArg1=0), 2) |
There was a problem hiding this comment.
This test confirms we're calling the right method, but doesn't verify that the arguments are passed in as expected. Ideally the result should incorporate the values from the arguments.
src/rubicon/objc/api.py
Outdated
| f"Known keywords are:\n" | ||
| + "\n".join(repr(keywords) for keywords in self.methods) | ||
| ) | ||
| f"Invalid selector {specified_sel!r}. Available selectors are: " |
There was a problem hiding this comment.
A minor cosmetic detail, but the quotes added by !r aren't needed here, because the identifier can't ever have spaces or anything else that might be ambiguous.
tests/objc/Example.m
Outdated
| +(void) dummyForException:(NSUInteger)arg | ||
| { | ||
| // Dummy method for testing exception | ||
| } |
There was a problem hiding this comment.
There's no need to have these extra methods just to demonstrate we can't call them correctly. The test will work just as well by calling Example.overloaded(0, notAnArgument=10). It would be arguably better to use overloaded:... , because there are more options, and some of them are ambiguous.
src/rubicon/objc/api.py
Outdated
|
|
||
| args = [] | ||
| rest = frozenset() | ||
| rest = (*order,) |
There was a problem hiding this comment.
This is technically correct, but isn't intuitive to read. I'd suggest creating the original order as a tuple comprehension; then this can be just rest = order, and the version in the else branch is rest = ("",) + order
changes/461.feature.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Updated the exception message when calling Objective-C methods with invalid keyword arguments to include selector names. | |||
There was a problem hiding this comment.
The error message has been improved when an Objective-C selector cannot be found matching the provided arguments.
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
|
Thanks for the reviews. I have made some changes as you say, and added a description to the documentation. |
There was a problem hiding this comment.
Thanks for those fixes. I've made a couple of additional tweaks to the documentation and change notes; but otherwise, this looks great!
Thanks for these fixes - the ambiguous argument problem has been one of those small annoying feature requests that I've wanted to fix for a while, but it's never been quite annoying enough to get to the top of my todo list.
docs/tutorial/tutorial-1.rst
Outdated
| ``__1`` and ``__2`` are added to avoid argument name collisions. Since ``__`` | ||
| and any part after it are ignored, it is possible to use any suffix starting | ||
| with it. | ||
|
|
There was a problem hiding this comment.
performSelector isn't a great example here, as it can be easily confused with the fact that we're discussing how to access selectors. Going "meta" in the tutorial is a potential source of confusion.
There's also the following paragraph, which contradicts this one; and the need for API documentation in addition to the tutorial.
|
Thank you! |
The partial method system was changed as follows:
PR Checklist: