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

Not using try! #26

Merged

loudmouth merged 1 commit into contentful:master from tapwork:master

Jul 21, 2017

Contributor

tapwork commented Jul 20, 2017 •

edited

Loading

Use a proper do catch or use try? in combo with guard or if let
and display the error to get more context in failure case.


          Not using try!

55fe9ae

Use do catch or use try? in combo with guard or if let

tapwork mentioned this pull request

Use of try! should be avoided #25

Closed

Contributor

loudmouth commented Jul 20, 2017

Hey @tapwork thanks for bringing this issue up and also for getting a fix together!

The try! issue has been in the back of my mind since I picked up this project, but as the persistence library is simply conforming to a protocol that the main SDK defines, I was unsure about how to bubble up thrown errors back through the SDK and to the client program. The messages to the PersistenceIntegration are obviously sent in an async callback from the network and I'd prefer to not require that every network callback throws as it will add unnecessary boilerplate for these callbacks in 90% of cases (i.e. when the consumer is not using a persistence integration).

I think the assertionFailure() is a good solution, but I'm curious if all fatalError() should be swapped for the assertionFailure() calls so that production applications won't crash...I'm not sure if it's preferred to crash the application on failure to create/update a CoreData entity.

Contributor

loudmouth commented Jul 20, 2017

Just in case my previous comment was unclear, I just wanted to hear your opinion on assertionFailure() vs fatalError() before merging.

Contributor Author

tapwork commented Jul 20, 2017 •

edited

Loading

hey @loudmouth
sorry for my late answer.
I also thought about fatalError vs. assertionFailure.
It should be this: fatalError only if the program can't really go on and assertionFailure if the program still could go on. I will update my PR. give 1-2 hours

tapwork commented

View reviewed changes

Contributor Author

tapwork left a comment

@loudmouth I didn't change the code. I think it might be okayish as it used to be. Can you have a look into my comments

Sources/SynchronizationManager.swift

+                              persistable = try self.persistentStore.create(type: type)
+                              persistable.id = asset.id
+                          } catch let error {
+                              fatalError("Could not create the Asset persistent store\n \(error)")

Contributor Author

tapwork Jul 21, 2017

I would leave fatalError here (so even crash in live world). There is no guarantee that the program will work properly without the model. (which is expected to exist). The creation of the model should only fail in really rare cases. If it fails something else is really broken.
@loudmouth what do you think?

Sources/SynchronizationManager.swift

+                              persistable.id = entry.id
+                              persistable.createdAt = entry.sys.createdAt
+                          } catch let error {
+                              fatalError("Could not create the Entry persistent store\n \(error)")

Contributor Author

tapwork Jul 21, 2017

Same here like in https://github.com/contentful/contentful-persistence.swift/pull/26/files#diff-6a7142dc6fe34b7d28c35b8871cad516R142

Sources/SynchronizationManager.swift

+                      do {
+                          try persistentStore.save()
+                      } catch let error {
+                          assertionFailure("Could not save the persistent store\n \(error)")

Contributor Author

tapwork Jul 21, 2017

The program can live without a save

Sources/SynchronizationManager.swift

                   // Returns regular (non-relationship) field to property mappings.
                   internal func propertyMapping(for entryType: EntryPersistable.Type, and fields: [FieldName: Any]) -> [FieldName: String] {
+                      // Filter out user-defined properties that represent relationships.
+                      guard let persistentRelationshipPropertyNames = try? persistentStore.relationships(for: entryType) else {
+                          assertionFailure("Could not filter out user-defined properties that represent relationships.")

Contributor Author

tapwork Jul 21, 2017

I am not 100% sure if this is really necessary to filter out the items in order to have a stable running app.
@loudmouth what do you think?

Sources/SynchronizationManager.swift

+                              let spacePersistable: SyncSpacePersistable = try self.persistentStore.create(type: self.persistenceModel.spaceType)
+                              return spacePersistable
+                          } catch let error {
+                              fatalError("Could not create the Sync Space persistent store\n \(error)")

Contributor Author

tapwork Jul 21, 2017

I think the space is needed for the whole model, right @loudmouth ?

loudmouth merged commit 9244106 into contentful:master

Contributor

loudmouth commented Jul 21, 2017

I think your approach is correct 👍

Just wanted to think about it. I'm going to merge now. Thanks again for the contribution! I'll add you to the changelog.

Contributor

loudmouth commented Jul 21, 2017

@tapwork I will probably not be publishing a new version for the next few days, so you will want to point to master in the meantime with your dependency manager.

Contributor Author

tapwork commented Jul 21, 2017

thanks @loudmouth
I will take master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment