-
Notifications
You must be signed in to change notification settings - Fork 976
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
Split bank account from params #761
Conversation
Similar to previous PR for STPCard. Also add STPSourceProtocol to STPBankAccount because it theoretically is a customer payment source as well.
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.
😄
|
||
/** | ||
A transfer sent to this bank account has failed. | ||
*/ |
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.
did documenting enums this way turn out to be better? i think we're flip-flop-ing on it.
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.
Yeah after playing around with it a bit, SourceKit (the Apple framework both Xcode and jazzy use for documentation) only recognizes enum docs written out this way, even though the Xcode doc generator doesn't do this. I'm gonna change everything to this as I audit the rest of our docs and will update our style guide.
*/ | ||
- (nonnull NSString *)last4; | ||
- (nonnull instancetype) init __attribute__((unavailable("You cannot directly instantiate an STPBankAccount. You should only use one that has been returned from an STPAPIClient callback."))); |
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.
we can drop the nonnull
here. also is it normal to have a space before init
when it's marked unavailable
?
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.
No, I think I just copy and pasted from somewhere else that had it wrong. I'll fix both things.
*/ | ||
@property (nonatomic, copy, nonnull) NSString *routingNumber; | ||
@property (nonatomic, nullable, readonly) NSString *routingNumber; |
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.
heheh finally corrected these 👏
@@ -49,31 +56,21 @@ + (STPBankAccountStatus)statusFromString:(NSString *)string { | |||
return STPBankAccountStatusNew; | |||
} | |||
|
|||
+ (NSString *)stringFromStatus:(STPBankAccountStatus)status { | |||
+ (nullable NSString *)stringFromStatus:(STPBankAccountStatus)status { |
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.
a little bit of a leap but I think we can offer nonnull
here?
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.
Ah yeah good point. Theoretically as long as you only pass in valid enum values it will always be nonnull, I'll fix.
@@ -127,7 +124,7 @@ + (NSArray *)requiredFields { | |||
]; | |||
} | |||
|
|||
+ (instancetype)decodedObjectFromAPIResponse:(NSDictionary *)response { | |||
+ (nullable instancetype)decodedObjectFromAPIResponse:(nullable NSDictionary *)response { |
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.
looking through the requiredFields
list, should we add:
- account_holder_type
- fingerprint
- status
to support our nonnull annotation?
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.
i guess this goes back to our discussion on how do we determine what fields we actually non-optional from the API
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.
I went the other way in most of this, where I treated whatever was in requiredFields
as currently correct, and tried to change the property declaration nullability to match. I might have missed a few, I can fix the header if needed. I think for now that is a better solution than changing requiredFields
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.
Wait, maybe I misunderstand your initial comment? Status is already listed as a required field. The other two are marked as nullable in the header, so I think this is correct already?
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.
Oh yeah, just double checked. I was looking at the property definitions in the implementation.
I think the only one out of sync is fingerprint
is nullable
in the header and nonnull (default)
in the implementation.
Also the accountHolderType
will default to STPBankAccountHolderTypeIndividual
if it's not defined in the response which is a little sketchy but probably the right fallback anyway.
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.
Yeah I agree that's sketchy, but not sure what to do about it.
I can fix the other nullability mismatch.
oh snap, the basic |
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.
👍
@@ -11,14 +11,21 @@ | |||
#import "NSDictionary+Stripe.h" | |||
#import "STPBankAccountParams+Private.h" | |||
|
|||
NS_ASSUME_NONNULL_BEGIN |
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.
🎉
* Sync strings * Add snapshot tests * Cleanup
Similar to previous PR for STPCard.
Also add STPSourceProtocol to STPBankAccount because it theoretically is a customer payment source as well.