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

Add missing exception objects #766

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Conversation

CCP-Aporia
Copy link
Contributor

As discovered in #765 there were two error conditions in _fsevents.add_watch that didn't set their exception status.

@CCP-Aporia
Copy link
Contributor Author

Not sure why the inotify test failed - is it flaky?

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 22, 2021

Thanks @CCP-Aporia 👍

Out of curiosity, do you know why v1.0.2 does not have such issue? Is it silently skipping the watch?

@CCP-Aporia
Copy link
Contributor Author

I'm not 100% certain, but up until - and including - version 1.0.2 we were silencing all Exception-based exceptions in FSEventsEmitter.run, so that's a likely reason for this having gone unnoticed for so long.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 22, 2021

That makes sense. Could you add a NEWs entry? Then we will be good to go.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 22, 2021

I would wait for @samschott review also, if possible :)

Copy link
Contributor

@samschott samschott left a comment

Choose a reason for hiding this comment

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

Looks good to me. And the reason why this didn't previously raise an exception makes sense.

src/watchdog_fsevents.c Show resolved Hide resolved
@BoboTiG BoboTiG merged commit 6dfb322 into gorakhargosh:master Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants