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.

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

Optional initialization #587

Merged
merged 3 commits into from
Mar 7, 2021
Merged

Conversation

vandadnp
Copy link
Contributor

@vandadnp vandadnp commented Feb 8, 2021

This PR addresses #586

These are breaking changes in that they return optional values from initializers instead of explicitly unwrapping the model objects. We should test these properly!

@vandadnp vandadnp marked this pull request as draft February 8, 2021 11:42
@vandadnp vandadnp changed the title Optional initialization and prevent try! and explicitly unwrapping return values Optional initialization Feb 8, 2021
@vandadnp vandadnp marked this pull request as ready for review February 8, 2021 12:56
@yarodevuci
Copy link

@3lvis Please approve this

@vandadnp
Copy link
Contributor Author

We should perhaps bump the version number to 7.0.0 to indicate a major and breaking change 💡

@vandadnp
Copy link
Contributor Author

@3lvis any updates on this please? thank you in advance

@3lvis
Copy link
Owner

3lvis commented Mar 4, 2021

Hi, sorry, I haven't been focusing on iOS apps for the last couple of years so my replies are usually slower than when I started the project.

I think this has a lot of correct but I get the feeling that this might not be the right approach. I still see some forced unwrapping so I get the feeling that we are moving the problem somewhere else.

@vandadnp
Copy link
Contributor Author

vandadnp commented Mar 4, 2021

@3lvis thanks for your response

All the explicitly unwrapped optionals that you can see in this PR are in the tests. The reasons I didn't optionally unwrap them in the tests is that if the situation arrises that the test suite cannot create a data stack, then the tests crash and fail. And that's the expected behavior from a test suite that it fails if it cannot proceed.

@vandadnp
Copy link
Contributor Author

vandadnp commented Mar 4, 2021

@3lvis regarding Sync and its future now that you're not focusing much on iOS development. Should there be a discussion in the Issues section about stewardship and the future of Sync? I think if this project is not going to be maintained by anybody, it's only fair to mention that right out in the README.md file so that projects out there like ours using Sync can start migrating to something else. What are your thoughts?

@3lvis
Copy link
Owner

3lvis commented Mar 7, 2021

@vandadnp that sounds sensible, I tried archiving all my projects but some people asked me to unarchive them since it was working for them.

@3lvis
Copy link
Owner

3lvis commented Mar 7, 2021

Had a second look at this, the principal challenge to fix this properly is Objective-C support. I can make a throwable initalizer that checks for the creation of the persistence coordinator which has a lot of try's inside but if I remove Objective-C we have to rewrite chunks of Sync since there's some Objective-C in here.

@3lvis
Copy link
Owner

3lvis commented Mar 7, 2021

Your solution on making it an optional initializer might be our best short term option.

@3lvis 3lvis changed the base branch from master to optional-initializer March 7, 2021 11:17
@3lvis 3lvis merged commit bd849b2 into 3lvis:optional-initializer Mar 7, 2021
3lvis added a commit that referenced this pull request Mar 7, 2021
This reverts commit bd849b2, reversing
changes made to 8789781.
@3lvis
Copy link
Owner

3lvis commented Mar 7, 2021

Doesn't seem like there's a way to reopen this issue.

First of all, I want to apologize for a lack of proper follow up. I have considered the possible cases of doing this as an optional, for example the fact that you wouldn't have been able to see that this was a lack of memory error.

If we go the optional initializer route the crashes will be avoided but the potential downside will be worse since your users won't be able to know what the problem was.

@3lvis
Copy link
Owner

3lvis commented Mar 7, 2021

I have updated the repository marking that it's no longer maintained, and I will reconsider archiving it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants