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.

Sign up for GitHub

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

Jump to bottom

Add FsEventWatcher Support. fix #7 #13

Merged
merged 8 commits into from
Jun 8, 2015
Merged

Add FsEventWatcher Support. fix #7 #13

merged 8 commits into from
Jun 8, 2015

Conversation

andelf
Copy link
Contributor

@andelf andelf commented Jun 8, 2015

This is ispired by the original work of https://github.com/octplane/fsevent-rust .

Currently you can only call .watch() once, and .unwatch() left unimplemented.
Good news is that Drop works. :)

UPDATE: .watch() .unwatch() works.

r: #12 #7
@octplane

@andelf andelf changed the title Add FsEventWatcher Support. Add FsEventWatcher Support. fix #7 Jun 8, 2015
@passcod
Copy link
Member

passcod commented Jun 8, 2015

I'm going to leave this to @octplane to evaluate and decide if it's mergeable or if some of it should be split back to fsevent-rust.

I'd also like to have watch and unwatch working completely before merging, unless y'all think that's too much work.

@andelf
Copy link
Contributor Author

andelf commented Jun 8, 2015

:) done.

@andelf
Copy link
Contributor Author

andelf commented Jun 8, 2015

@octplane if some of this should be split back to fsevent-rust?

@octplane
Copy link
Contributor

octplane commented Jun 8, 2015

LGTM. I will probably later remove some code from here to avoid duplication with my binding, but is seems reasonable to merge as-is if it is working correctly :). Great job @andelf!

@octplane
Copy link
Contributor

octplane commented Jun 8, 2015

@andelf: yes ! your comment arrived while I was writing mine :)

@andelf
Copy link
Contributor Author

andelf commented Jun 8, 2015

@octplane @passcod
This PR can be merged and @octplane and I will fix the duplicated code betweent fsevent-rust and rsnotify. ;)
The API will be compatiable.

BTW, what's the suitable behavior of after unwatch() all paths in a Watcher? Stop the Watcher or triger an error?

@passcod
Copy link
Member

passcod commented Jun 8, 2015

Leave the Watcher open for further watch() calls. If it's best to spin down the inner implementation or somesuch, do that (and spin it back up on a .watch()), but as long as it's not Dropped I would expect to be able to use it.

@passcod
Copy link
Member

passcod commented Jun 8, 2015

I'll probably merge in either a few hours, or tomorrow. Thanks both for the great work!

passcod
passcod reviewed Jun 8, 2015
View reviewed changes
src/lib.rs
@@ -67,7 +75,18 @@ fn new_inotify() {
}

#[test]
fn new_poll() {
#[cfg(target_os = "macos")]
fn new_inotify() {
Copy link
Member

Choose a reason for hiding this comment

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

That should say new_fsevent().

@passcod
Copy link
Member

passcod commented Jun 8, 2015

Comments in the code are minor style nitpicks, ignore them; I'll fix in the merge.

passcod added a commit that referenced this pull request Jun 8, 2015
@passcod
Merge pull request #13 from andelf/fsevent-support
32ff762 
Add FSEvent support
Fix #7
Also see #12
@passcod passcod merged commit 32ff762 into notify-rs:master Jun 8, 2015
passcod
passcod reviewed Jun 8, 2015
View reviewed changes
src/fsevent.rs
}


pub fn is_api_available() -> (bool, String) {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be a Result<(), String>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andelf @passcod @octplane