Tighten API of send_message #174
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This feature is rarely used - it had only a single usage in the rubicon-objc codebase, and is probably even less used outside, because most code can use high-level ObjCClass method syntax instead of send_message. The few cases that used this behavior can be easily updated to use an explicit get_class or ObjCClass call instead, so there's not much benefit in having this special case in send_message.
restype defaulted to c_void_p, which is very rare as a return type for Objective-C methods. Most calls that use this default should actually use None (void) or objc_id instead, and those that really need a c_void_p restype can set it explicitly. argtypes defaulted to an empty list, which made it easy to accidentally pass all arguments using ctypes default argument conversions, which can lead to unexpected/incorrect type conversions depending on the method signature and the actual passed arguments. It's better to always explicitly list all argument types to ensure that they are converted and passed correctly.
Objective-C object pointers should always be stored as objc_id or a similar type, to avoid accidentally treating random pointers as Objective-C objects. The implicit cast from c_void_p was no longer used anywhere in the rubicon-objc codebase, as all of our APIs return Objective-C object pointers properly typed.
Previously send_message accepted more arguments than are specified in argtypes. This is useful when calling variadic methods, but is easy to misuse by accident, because extra arguments passed to non-variadic methods are silently ignored. Varargs now need to be passed as a list into a separate kwarg, which allows checking the number of non-variadic args against the number of argtypes.
The three branches were almost entirely identical - most of their contents can be pulled out of the branches.
freakboy3742
requested changes
Apr 27, 2020
There was a problem hiding this comment.
Yes, this is opinionated, but it helps when you have good opinions :-)
There's nothing here that particularly concerns me - it looks like a good tightening of arguments where it is entirely appropriate to do so, and a clarification of variadics in a way that removes a potential footgun.
The only real tweaks I can see are related to documentation - the formatting of the removal note, and some clarification around types in variadics. Adding something to the API docs about variadic handling would also be a nice touch.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is a little opinionated - it removes a couple of special features from
send_messagethat, in my opinion, are either too error-prone or too rarely used to be worth the extra complexity. See the individual commits for explanations on why I think it would be best to remove each of these features.I'm open to feedback on these changes. If you think any of these features are legitimately useful and removing them would be a bad idea, please let me know.
Also, this is obviously a breaking change - it removes features that were previously documented and supported. As we're still before version 1.0, this is technically fine. I don't think any of these features are widely used enough to require a proper deprecation process, but I'm open to feedback here as well.