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

dispatch queue #215

Merged
merged 1 commit into from
Jan 12, 2023
Merged

dispatch queue #215

merged 1 commit into from
Jan 12, 2023

Conversation

josharian
Copy link
Contributor

@josharian josharian commented Dec 14, 2022

No description provided.

@josharian
Copy link
Contributor Author

josharian commented Dec 14, 2022

This is built atop #214, which isn't in yet; i will rebase once that goes in.

I've asked a friend to help review this, to make sure I haven't screwed up anything obvious. Tests pass locally, with and without the race detector, but that doesn't necessarily mean it is correct. :)

This simplifies the code and removes a runloop-related deprecation.

Fixes rjeczalik#212
@kevboh
Copy link

kevboh commented Dec 14, 2022

@josharian with the caveat that I'm not familiar with FSEventStreamSetDispatchQueue beyond reading through the docs, this looks correct to me.

@josharian josharian marked this pull request as ready for review December 14, 2022 15:36
@josharian
Copy link
Contributor Author

OK, @rjeczalik, all yours for review, at your leisure. :)

@rjeczalik
Copy link
Owner

@josharian Sorry for letting it hang, holiday period and having less spare time than usual, I will get back to this some time around next week scout's word 🤞

@josharian
Copy link
Contributor Author

Ping.

Copy link
Owner

@rjeczalik rjeczalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josharian looks good, thank you very much for this PR! I have one q though about calling FSEventStreamSetDispatchQueue in Stop()?

@rjeczalik rjeczalik merged commit bc7f94f into rjeczalik:master Jan 12, 2023
@rjeczalik
Copy link
Owner

@josharian Thanks! ❤️

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

Successfully merging this pull request may close these issues.

None yet

3 participants