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

[FIX] Setting PERSISTENCE = YES #444 #445

Merged
merged 1 commit into from
Apr 24, 2018
Merged

[FIX] Setting PERSISTENCE = YES #444 #445

merged 1 commit into from
Apr 24, 2018

Conversation

ckrey
Copy link
Contributor

@ckrey ckrey commented Apr 5, 2018

The problem can be solved by lazy initialization of the NSPersistentStoreCoordinator

@jcavar
Copy link
Contributor

jcavar commented Apr 6, 2018

Interesting, thanks @ckrey. Would it be possible to add some tests around this?

@jcavar
Copy link
Contributor

jcavar commented Apr 6, 2018

Also, are maybe some other issues related to this?

@ckrey
Copy link
Contributor Author

ckrey commented Apr 6, 2018

Checks are difficult, because you see the results of persistence / non persistence only when you kill and restart the App in between...

I don't see any other open issue related to this. The CoreData persistence works exactly the same whether it is memory based or backed by SQLLite except is does not survive a kill app.

The persistence worked fine probably until 7 months ago when you solved a lot of concurrency problems by doing this: (this actually disabled the PERSISTENCE = YES setting)

bildschirmfoto 2018-04-06 um 16 30 37

@jcavar
Copy link
Contributor

jcavar commented Apr 6, 2018

Gotcha, but we could add test where we initialise MQTTSession set persistence to YES or NO and assert that store type is NSSQLiteStoreType or NSInMemoryStoreType. This test would catch if someone tries to do something like I did in past?

@jcavar
Copy link
Contributor

jcavar commented Apr 24, 2018

I've added those tests here:
#454

so I will merge this.

Thanks @ckrey!

@jcavar jcavar merged commit bc31737 into novastone-media:master Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants