-
Notifications
You must be signed in to change notification settings - Fork 222
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: PollWatcher #409
Conversation
9ebfe95
to
9bca8ef
Compare
Refactor PollWatcher to make it clean for further develop. This commit would not change any behaviors user observable (not include Debug implementation) and public documentation.
9bca8ef
to
f128369
Compare
f146e9a
to
574c7a5
Compare
Found a different behavior: In See:
After refactor this behavior was gone. I think it's a bug so not worth to add it back, only note it here. |
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.
Thanks! This looks very good.
Sometimes I'm not quite sure when cargo fmt does accept something or doesn't..
fn watch_inner(&mut self, path: &Path, recursive_mode: RecursiveMode) { | ||
// HINT: Make sure always lock in the same order to avoid deadlock. | ||
// | ||
// FIXME: inconsistent: some place mutex poison cause panic, some place just ignore. |
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.
I'd rather we throw this back to the user or decide on a way to handle it.
Ignoring isn't something we want to do, except if we note why.
Refactor
PollWatcher
to make it clean for further develop.This commit would not change any behaviors user observable (not include
Debug
implementation) and public documentation.I also add some
QUESTION
andFIXME
in code comment to descript some weird (IMHO) behaviors. (Maybe we can fix it, or they have some pretty good reason so we should add description beside them)If need any further change, please let me know, thanks.