-
Notifications
You must be signed in to change notification settings - Fork 35
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
Make PayPalError and CardError Code enum consistent with error naming pattern #298
Conversation
@@ -39,7 +39,7 @@ public enum CardError { | |||
case malformedDeeplinkURLError | |||
|
|||
/// 10. Cancellation from 3DS verification | |||
case threeDSCancellationError | |||
case threeDSecureCanceledError |
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 is a random side nit: I noticed the Error
suffix may be a bit redundant here since the type is CardError
. I wonder if it's worth cleaning up in a future release.
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.
Yes, I thought about that, and that was my initial reason for changing it in the static function name but noticed that there was enum and static function name match and didn't want to deviate just for these.
What I want for next release is to be able to do if error == CardError.threeDSecureCanceled
or if error == PayPalError.checkoutCanceled
.
Or something hierarchical like error.source == .card(.threeDSecureCanceled)
if we keep the CoreSDKError
<= this seems more cumbersome than what I have here though
I think it's matter of preference and depends on if it's something merchant would act on.
I can definitely see merchants wanting to separate cancel case as I did in the demo app.
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.
Got it yeah. I think the Swift API Guidelines have a naming convention for factory methods if we want to go with something like makeThreeDSecureCanceledError()
for the internal method in the future. But it's super nit. I do think brevity in the case
name (within reason) is a big win for developer experience.
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 think the first one is more aligned with what I see on Android.
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 that definitely will change on Android. They shouldn't have been named that way.
* Remove MXO (#291) * Remove MXO * remove Package.resolved file containing incompatible pinstorage version * CHANGELOG entry * Jax feedback and docStrings correction * Remove PatchOrder for MXO, unused files in demo app * Remove more classes used exclusively for MXO * Convert Delegation Pattern to Completion Handler (#293) * PayPal remove delgation pattern for completion handler function for checkout * lint error for paypal start * completion and analytics into notify functions * paypal vault completion, unit tests * remove PayPal delegates, references * Card vault to completion, unit tests * Card approve to completion, unit tests * docstrings for approve completion param * wrap paypal functions in async await * async await wrapper for CardClient functions * remove CardDelegate and references * fix error in notifyCheckoutFailure * Make error names and messages payPal caps consistent * changelog entires * Steven PR feedback: typo in Chagelog * Simplify Cancel Errors (#296) * Simplify cancel errors * CHANGELOG for the cancel errors * Steven PR feedback: change back CardClientError.canceled to .threeDSecureCanceled * CHANGELOG update * Cancel error handling (#297) * http performRequest returns NetworkingClientErrors * CardClient helper function for threeDSCancel, demo app cancel, demo app minor fixes * PayPalClient cancel helper functions and demo app changes * Steven PR feedback: move static helper functions to error enums * Rename CardClientError -> CardError, PayPalWebCheckoutError -> PayPalError * Steven PR feedback: return CoreSDKError in merchant completion handler * CHANGELOG and analytics typo and fix wrong code in graphql error * v2 migration guide (#295) * v2 migration guide * just cocoapods or SPM * fix typos * minor spacing changes * Update with simplified cancel errors * Steven PR feedback - diff to render green/red * include removal of delegate methods in delete block * update with cardClient threeDSecureCanceled error * change to threeDSecureCanceled in migration steps * add comment highlighting cancellation errors * typo fix * clarify separating cancel cases in errors * revert cancel handling instructions * add changes for cancellation helper methods * fix typo in PayPalError.isCheckoutCanceled * Steven PR feedback * Make PayPalError and CardError Code enum consistent (#298) * Changelog: move additive changes from breaking section (#299) * Changelog: move additive changes from breaking section * remove duplicate line for PayPal cancel errors
Summary of changes
Checklist
Authors