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

Remove the watchdog/observers/fsevents2.py file #905

Closed
wants to merge 2 commits into from

Conversation

kurtmckee
Copy link
Contributor

The file is never imported, is untested, and is unmaintained since 2014 (excepting changes caused by broad project fix-up events, like fixing flake8 warnings or dropping Python 2.7 support).

The file is never imported, is untested, and is unmaintained since 2014
(excepting changes caused by broad project fix-up events,
like fixing flake8 warnings or dropping Python 2.7 support).
@BoboTiG
Copy link
Collaborator

BoboTiG commented Jul 8, 2022

Hmm that's quite a clean-up here. I think fsevents2 may be used by some projects, but I do not have any numbers.

I would be +0 to remove it, not a strong opinion.

What are your thoughts @CCP-Aporia, and @samschott?

@samschott
Copy link
Contributor

samschott commented Jul 8, 2022

I'm not sure if this module is actually in use (I'm not using it). However, I can image it being important on platforms or Python versions where PyObjC provides wheels and watchdog does not.

IMO, the better option would be to mark it as unsupported in the docs, or even as deprecated if we don't plan on porting over the improvements made to fsevents. This would allow users to migrate in their own time.

@kurtmckee
Copy link
Contributor Author

I used git grep fsevents2 $(git rev-list --all) to hunt for references across the entire git history and found a blip where fsevents2 is mentioned in the changelog (though the note was rephrased later to remove that mention):

My goal in creating this PR is to remove dead code; it's undocumented, untested, unmaintained, and cannot be imported without installing unlisted dependencies. I have no issue with this PR getting rejected, but I recommend choosing whether to support/deprecate/delete the code. 👍

@kurtmckee
Copy link
Contributor Author

I found what appears to be a helper script related to fsevents2 (based on its FSEvents import).

@CCP-Aporia
Copy link
Contributor

It is certainly odd to have fsevents and fsevents2. Our project only uses fsevents.

Maybe the first step is to add a DeprecationWarning upon import, and then drop it a few months later if nobody comes up with a use-case?

@kurtmckee
Copy link
Contributor Author

kurtmckee commented Jul 11, 2022

Sounds like a DeprecationWarning on import is step 1, then at some point in the future fsevents2 can be deleted.

I'll create a PR to introduce a DeprecationWarning and tag you three on it for review. Thanks for the feedback!

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.

None yet

4 participants