fixes and improvements related to Core Data usage #1443 #1471
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryanmontz
requested review from
mplorentz,
martindsq and
joshuatbrown
as code owners
mplorentz
approved these changes
Sep 9, 2024
martindsq
requested changes
Sep 9, 2024
joshuatbrown
approved these changes
Sep 9, 2024
# Conflicts: # CHANGELOG.md # Nos/Models/CoreData/Event+CoreDataClass.swift
|
@martindsq I think in this case posting your review as a Comment rather than Request Changes would have sped things along. Right now merging is blocked waiting on your approval even though you said you were fine with all changes. I know we have had some debate about when to use Request Changes vs Comment so I wanted to point this out. If we had folks who just clicked merge and forgot to come back and finish the "optional" conversations then I don't think Comment would be a good idea. But I think we have a team who stays on top of their Github conversations which is great, so I think we can trust that in general. |
martindsq
approved these changes
Sep 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues covered
#1443
Description
These changes are the result of the Core Data usage research prescribed by #1443. They are the changes I was confident making without further discussion. Other suggestions will be mentioned in the ticket.
This code:
Event().createIfNecessary(jsonEvent...is creating Events and throwing them away immediately and causing the console error message "Failed to call designated initializer on NSManagedObject". The resolution is to make thecreateIfNecessary...function static.Adding the launch argument
-com.apple.CoreData.ConcurrencyDebug 1reveals thatEvent.loadFirstQuotedNote()is creating an event on the wrong thread. The resolution is to move theEvent.findOrCreateStubBy...call to inside thecontext.performcall below. Reference https://useyourloaf.com/blog/debugging-core-data/.In
RelayService.retryFailedPublishes(), a JSONEvent was being created inside a nested loop. This caused needless extra work because it could be created only in the outer loop.How to test
These changes aren't user testable, but they are dev testable.
Running
main(prior to the changes in this PR), you will see a message in the console like this:"Failed to call designated initializer on NSManagedObject"
If you add the launch argument
-com.apple.CoreData.ConcurrencyDebug 1to the scheme and run the code, you will get a crash (an assert really) if you scroll around in a feed view that contains quoted notes.After the changes in this PR, both of those issues are resolved.