Skip to content
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.

Sign up for GitHub

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

Jump to bottom

New ErrorHandler class #15

Merged
merged 13 commits into from
Dec 15, 2017
Merged

New ErrorHandler class #15

merged 13 commits into from
Dec 15, 2017

Conversation

randycarney
Copy link
Contributor

Implemented a new class called "CKErrorHandler" to switch on the generated error code and handle any and all potential CKError.codes should they arise.

I modified SyncEngine to use the CKErrorHandler class where appropriate.

@randycarney
Implemented a new class CKErrorHandler to appropriately handle all po…
ce10309 
…tential CKError.codes as they arise

Modified SyncEngine code to use CKErrorHandler class where appropriate
@caiyue1993
Copy link
Owner

Thanks @randycarney! I've checked your code and it seems great!

But it takes time for me to review it in detail. And I am gonna tell you my thoughts on this later.

Thank you again!

@caiyue1993
Copy link
Owner

caiyue1993 commented Dec 13, 2017 •
edited
Loading

Reference issue #13

@insidegui
Copy link

Is it a good idea to use CK as a prefix for our types?

That could cause conflicts in the future if Apple changes their API with types which have the same names.

@randycarney
Copy link
Contributor Author

Fair point, I wasn't sure what to name it! CloudKitErrorHandler? ErrorHandler?

@randycarney
Renamed CKErrorHandler to ErrorHandler
0f7c221 
Replaced ErrrorMessageForUser struct as a simpler String type. Instead, added a message to certain ErrorTypes that could be displayed to the user.

Re-classified network-related failure codes as ErrorType.recoverableError s(user should take action) instead of retry

Re-classified func retryOperationIfPossible(retryafter:block) as type method (“static”)

Moved Array extension for chunking to ErrorHandler file

- Cleaned up Header
@randycarney
Renamed CKErrorHandler to ErrorHandler
82fc820 
Replaced ErrrorMessageForUser struct as a simpler String type. Instead, added a message to certain ErrorTypes that could be displayed to the user.

Re-classified network-related failure codes as ErrorType.recoverableError s(user should take action) instead of retry

Re-classified func retryOperationIfPossible(retryafter:block) as type method (“static”)

Moved Array extension for chunking to ErrorHandler file

- Cleaned up Header
@insidegui
Copy link

I think going without a prefix is the best approach here.

@caiyue1993
Copy link
Owner

Hi @randycarney, I've made some changes and code has become much cleaner and reasonable now.
I have one question, why you made retryOperationIfPossible a static method? I think a instance method should be OK.

Hope to receive your comments.

@caiyue1993 caiyue1993 mentioned this pull request Dec 15, 2017
Closed
@randycarney
Copy link
Contributor Author

This is great, nice job. Fine for retryOperationIfPossible to be an instance method.

@caiyue1993
Copy link
Owner

caiyue1993 commented Dec 15, 2017 •
edited
Loading

@randycarney Yep, I've made it an instance method. Time to merge.

@caiyue1993 caiyue1993 merged commit f01a770 into caiyue1993:master Dec 15, 2017
@randycarney
Copy link
Contributor Author

My first pull request! 💯

@randycarney randycarney deleted the CKErrorHandler branch December 15, 2017 17:24
@caiyue1993
Copy link
Owner

Congrats man! Seems that you've learned so fast. But I have some tips for you:

  1. Keep each commits focused which means normally you should separate a big commit into smaller ones. That will help maintainers to review code and easy to understand.
  2. Before raising a pull request, do the test yourself in advance.
  3. Naming matters.

You can do better in the future! Thanks!

@randycarney
Copy link
Contributor Author

Awesome, thanks for the advice. I learned a lot from your edits too!

@insidegui
Copy link

@randycarney Congrats on your first pull request 🎉

@caiyue1993 caiyue1993 changed the title New CKErrorHandler class New ErrorHandler class Dec 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@randycarney @caiyue1993 @insidegui