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

Closures called on main queue #17

Closed
rodericj opened this issue Feb 10, 2017 · 5 comments
Closed

Closures called on main queue #17

rodericj opened this issue Feb 10, 2017 · 5 comments

Comments

@rodericj
Copy link

I noticed that the closures for completion come back on the main queue. I was hoping to have the callbacks on a background queue but I don't see any infra for that in the code or documentation. Is there a reason for this? If I were to just remove the DispatchQueue.main.async would you anticipate any adverse effects?

@Jeehut
Copy link
Member

Jeehut commented Feb 12, 2017

The reason the callbacks are made in the main thread is that this library was written for cases where you need to import a (potentially) large CSV file and want to give the user some kind of progress feedback. That's what the onProgress and onFinish callbacks were designed for and since the UI should only be updated in the main thread, those blocks are called on the main thread.

I don't see any adverse effects except for the above mentioned UI changes when you would fork this and remove the main thread calls. If you want to ensure that you have an up-to-date framework, feel free to make your change in your fork additive (and non-breaking) so I can merge your changes via a Pull Request and we have optional support for explicitly defining the thread on which the callbacks should be called. I imagine something like this:

let importer = CSVImporter<[String]>(...)
importer.qosClass = .background
importer.startImporting(...).onProgress {
    // called in background thread
}.onFinish {
    // called in background thread
}

By default it would then still make the calls in the main thread, except for if you have defined a qosClass, then it would use a global DispatchQueue of the given qos class.

@rodericj
Copy link
Author

I think that's reasonable. I am importing a large CSV but I'd like to have all processing finished on the background thread where I import into my indexable data model. While I love your implementation I may end up rolling my own or using one of the other parsers. My need is slightly different than what you're addressing.

Thanks for getting back to me.

@Jeehut
Copy link
Member

Jeehut commented Feb 13, 2017

But the importing is done on a background thread and adding support for background threads in the callbacks is not a complex task, if you're not sure how to add that feature yourself I can do that. Others may need the feature, too.

@Jeehut Jeehut reopened this Feb 13, 2017
@Jeehut
Copy link
Member

Jeehut commented Feb 13, 2017

I just implemented such a feature and released version 1.6.0 with it included. See the end of the Asynchronous with Callbacks section in the README for usage. Your requested behavior should now be possible @rodericj. If you have any more questions feel free to ask.

Closing this.

@Jeehut Jeehut closed this as completed Feb 13, 2017
@Jeehut
Copy link
Member

Jeehut commented Feb 13, 2017

Note that I've also just added a synchronously working import method and released version 1.7.0 with it. Refer to the docs here.

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

No branches or pull requests

2 participants