-
Notifications
You must be signed in to change notification settings - Fork 8
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
refactor: Common download operation #1370
refactor: Common download operation #1370
Conversation
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
One question, I'm a bit on the fence regarding the @unchecked Sendable
for:
- DownloadOperation
- DownloadArchiveOperation
The other operations are fine because they don't have additional mutable state. However in the case of DownloadArchiveOperation
we rely on the task that urlSession should be thread safe and calls in a queue downloadCompletion
and assigns archiveUrl
but nothing protects archiveUrl from being assigned elsewhere.
I won't request changes but this is something we have to keep in mind when using @unchecked
Discussed regarding the |
We can push the refactor further and share the boilerplate of all
DownloadOperations
.I did not rewrite old code that might need further rewrite. This is just code factorisation.
Intended to be reviewed per commit.
Root PR: #1311