-
Notifications
You must be signed in to change notification settings - Fork 27
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.
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
Add user _id to createTrip #329
base: master
Are you sure you want to change the base?
Add user _id to createTrip #329
Conversation
@lmeier it looks like trip options is just set to the one passed in the
|
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 like we need to update the RadarTrip
class to include scheduledArrivalAt
and parse it from the JSON payload
@@ -25,7 +25,9 @@ NS_ASSUME_NONNULL_BEGIN | |||
- (instancetype)initWithExternalId:(NSString *_Nonnull)externalId | |||
destinationGeofenceTag:(NSString *_Nullable)destinationGeofenceTag | |||
destinationGeofenceExternalId:(NSString *_Nullable)destinationGeofenceExternalId | |||
scheduledArrivalAt:(NSDate *_Nullable)scheduledArrivalAt; | |||
scheduledArrivalAt:(NSDate *_Nullable)scheduledArrivalAt |
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 seems like a breaking change, would we need migration docs?
A follow up question is, do we want to keep on exposing these "init by fields" functions when we already expose the "init by dictionary" functions as we revamp the SDK for one line implementation? The upside would be that we will no longer be collecting deprecated interfaces. The downside would be that initializing from dictionary in native is kinda annoying and not developer friendly. However, one may also claim that with one line implementation all of these options that developers are currently building and loading into the client should be loaded in from the server.
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.
Correct, we shouldn't break this here. I'll either introduce another option with the 2 additional params or create a dictionary based initialization.
…p-and-update-tripoptions-with-trip
We actually don't need any change for setting the trip to the one returned as we already have that: https://github.com/radarlabs/radar-sdk-ios/blob/liammeier/fence-1902-send-user-on-starttrip-and-update-tripoptions-with-trip/RadarSDK/Radar.m#L491-L492