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

Only register directories for watching #74

Merged
merged 1 commit into from
Jun 8, 2016

Conversation

vandenoever
Copy link
Contributor

Directories also report the events for the files in the directories.

@passcod
Copy link
Member

passcod commented Jun 5, 2016

Walk me through this. What does your change do? Does it still support calling add_watch_recursively on a file?

@vandenoever
Copy link
Contributor Author

For normal files there is no need for a watch, so for files instead of calling add_watch, Ok(()) is returned.

@vandenoever
Copy link
Contributor Author

I see now that this patch does not yet support calling add_watch_recursively to register a watch on a single file.

@passcod
Copy link
Member

passcod commented Jun 6, 2016

This function originally has two scenarios:

  • If Path is a file, watch it
  • If Path is a dir, watch dir, all subdirs and subfiles

We want:

  • If Path is a file, watch it
  • If Path is a dir, watch dir and all subdirs only

Your patch does, even the updated version:

  • If Path is a file, fail silently
  • If Path is a dir, watch dir, all subdirs and subfiles

So, see, what your patch does is silently break a valid use case, and fail to actually address the problem.

@vandenoever
Copy link
Contributor Author

vandenoever commented Jun 8, 2016

I uploaded a new diff in pull request #76 . I'm on a very bad connection and have a hard time updating the diff on this PR.

Update: I managed to update PR #74 and will close PR #76

Directories also report the events for the files in the directories.
@passcod passcod merged commit 00375e8 into notify-rs:master Jun 8, 2016
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.

2 participants