Remove the function that finds posts with upload failure #19353
Conversation
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
There was a problem hiding this comment.
Upon reading the description, I thought "well, even if it's used only once, it might still be useful to have the logic isolated to make the code more readable."
After looking at the diff, I agree with your assessment that this narrow logic can suitably live within that method alone.
The only objection that crossed my mind is that it would be nice to have a consistent approach to running queries. I quite like what you have been doing with extracting queries into Active-Record-resembling extensions on model objects. I think it might be beneficial to make that the only way to run queries, so that we have a consistent pattern and abstraction across the codebase. But, that's a broader discussion that would only slow down merging this PR.
|
|
||
| init(_ managedObjectContext: NSManagedObjectContext) { | ||
| postService = PostService(managedObjectContext: managedObjectContext) | ||
| self.managedObjectContext = managedObjectContext | ||
| } | ||
|
|
||
| func postsAndRetryActions(result: @escaping ([AbstractPost: PostAutoUploadInteractor.AutoUploadAction]) -> Void) { |
There was a problem hiding this comment.
Unrelated to this PR, but I find this name confusing. Do you?
The result parameter is confusing as well. I can see from the type that it's a closure to execute on the results, but upon reading the name I couldn't help but thinking the function itself expected to be passed some kind of result.
I'm also always confused by anything called "result" that is not a Result<Value, Error> type, but that might be my bias towards that lovely type.
Anyway, just wanted to run it past you. I don't have a good suggestion for how to update the code.
There was a problem hiding this comment.
Agreed. Something like completion seems like a conventional name. I've seen a couple of other places that use the same variable name.
|
Took over merging this to ensure it's part of the 20.9 code freeze, which I'll run later today. |
I had actually attempted to build something like this based on this |
The main purpose is further simplifying
PostServiceclass. The original function was only used in one place. I removed it since I don't see the need of extract the logic into its own function, considering the function did a very specific, which is finding posts that failed to be saved onto the server and sounds like appropriate to be consolidated intoFailedPostsFetcher.Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
None. The new Swift implementation is a direct translation of the original Objective-C implementation.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txtif necessary.