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

Fixed the Event of watch dir Deletion #540

Merged
merged 8 commits into from
Nov 1, 2023
Merged

Fixed the Event of watch dir Deletion #540

merged 8 commits into from
Nov 1, 2023

Conversation

zeroishero
Copy link
Contributor

@zeroishero zeroishero commented Oct 7, 2023

Resolves #493
Deleting watched dir only returns Delete self and Ignore event. Unlike deleting other folders, it doesn't return events to check whether it is dir or not.

I stored whether path is dir or not in watches. Taking metadata after delete event triggers is not possible.

notify/src/inotify.rs Outdated Show resolved Hide resolved
notify/src/inotify.rs Outdated Show resolved Hide resolved
Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

Thanks! I've added some comments, let me know if you want to leave them to me or refactor those parts by yourself.

@zeroishero
Copy link
Contributor Author

I added the comment for watches HashMap.

@0xpr03
Copy link
Member

0xpr03 commented Oct 16, 2023

Any reason to close it ? I did ask for another thing: The unwrap you're using seems like something that could crash. Otherwise the PR is fine.

@0xpr03 0xpr03 reopened this Oct 16, 2023
@zeroishero
Copy link
Contributor Author

@0xpr03 are there more changes I should do before it gets approved?

@0xpr03
Copy link
Member

0xpr03 commented Nov 1, 2023

Looks good, I'm just too busy right now and don't wanna rush things.

if path.is_none() {
remove_kind = RemoveKind::Other
} else {
let watched_path = path.clone().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this now: When does this unwrap fail ? Do we need to clone here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

considering it's in else branch which checks for is_none condition, it shouldn't fail on unwrap.
The clone is there coz it needs to be owned by Event but path is in notify.

@0xpr03
Copy link
Member

0xpr03 commented Nov 1, 2023

Huh so apparently github never send my review.. Sorry for that..

@0xpr03 0xpr03 merged commit c4d0470 into notify-rs:main Nov 1, 2023
13 checks passed
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.

Delete a directory which is watched returns Event { kind: Remove(File) }
2 participants