Enable sending complete JSON payloads, as proxy for other SDKs #155
Conversation
Rollbar/Rollbar.h
Outdated
| @@ -80,6 +80,10 @@ | |||
|
|
|||
| + (void)logCrashReport:(NSString*)crashReport; | |||
|
|
|||
| // Send JSON payload | |||
|
|
|||
| + (void)send:(NSData*)payload; | |||
There was a problem hiding this comment.
it may make sense to rename it to:
+ (void)sendPayload:(NSData*)payload;
so it is in sync with the RollbarNotifier addition...
There was a problem hiding this comment.
I thought about that. The tradeoff is send is more consistent with the naming that is (now) introduced across the other SDK interfaces. The internal notifier naming differentiates sendItem and sendPayload. I suppose we could push that internal detail up to the public interface here. I was inclined to prefer the simpler send as long as it wouldn't be ambiguous in any context.
There was a problem hiding this comment.
I should add that in some SDKs (Java), Payload is a type and therefore sendPayload infers more that we would want. To the extent we want this to be consistent, like we do with log(), that's a reason to avoid sendPayload in the public interface.
There was a problem hiding this comment.
we should be consistent across the SDKs semantically as much as possible, but necessarily syntactically. for example, in .NET/C# all the public methods are capitalized by convention and not all languages support method overloads (objective-c is somewhat different here: https://learning.oreilly.com/library/view/ios-6-programming/9781449342746/ch01s15.html )
There was a problem hiding this comment.
Cocoa guidelines (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html) say sendPayload is probably the preferred name, although I hate quite a few of their guidelines.
|
|
||
| @return YES if successful. NO if not. | ||
| */ | ||
| - (BOOL)sendPayload:(NSData*)payload; |
There was a problem hiding this comment.
@waltjones , I assume you are just stubbing it out since there is no implementation for this method?
There was a problem hiding this comment.
Implementation is here: https://github.com/rollbar/rollbar-ios/blob/master/Rollbar/RollbarNotifier.m#L514
There was a problem hiding this comment.
@waltjones, i see. you essentially making it public here. missed that one...
waltjones
changed the title
The public interface has been updated to use
sendJsonPayload()instead ofsend(). This is meant to be consistent with rollbar/rollbar.js#724 as well as address the comments here.