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

Use the standard pathlib instead of pathtools #704

Merged
merged 1 commit into from
Nov 28, 2020

Conversation

bstaletic
Copy link
Contributor

Now with docstrings and tests. To be honest, I have not actually ran the tests and am hoping CI will be green and I won't have to.
The tests are taken directly from pathtools' patterns.py, so those should work.

One thing to note is the license. I have no idea if I'm supposed to copy/paste the complete license notice and if I'm supposed to attribute it to the original author. Just a thing to check...

Finally, to answer this question (which I did, but never hit "post"), that was intentional, though the reason is not immediately obvious. pathlib doesn't have a case_sensitive parameter anywhere in its API - PurePosixPath and PosixPath are always case-sensitive and PureWindowsPath and WindowsPath are always case-insensitive. On top of that, only "pure" path objects can be instantiated anywhere - WindowsPath can't be instantiated on POSIX systems and vice versa.

This lead me to "emulating" the case_sensitive parameter, by converting the input paths (as strings) to either PurePosixPath or PureWindowsPath, to satisfy case_sensitive.
In short, it allows this:

>>> import pathlib
>>> p = pathlib.PureWindowsPath('/etc/hosts')
>>> p.match('*hOsTs')
True
>>> p
PureWindowsPath('/etc/hosts')
>>> p = pathlib.PurePosixPath('/etc/hosts')
>>> p.match('*hOsTs')
False

Copy link
Collaborator

@BoboTiG BoboTiG left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the patch :)

Could you add a line in the changelog + your nickname?

src/watchdog/utils/patterns.py Outdated Show resolved Hide resolved
src/watchdog/utils/patterns.py Show resolved Hide resolved
tests/test_patterns.py Outdated Show resolved Hide resolved
tests/test_patterns.py Outdated Show resolved Hide resolved
tests/test_patterns.py Outdated Show resolved Hide resolved
@BoboTiG
Copy link
Collaborator

BoboTiG commented Nov 27, 2020

And I did not say it, but this is a clever usage of subclasses to emulate case-sensitivity 💪

@BoboTiG BoboTiG linked an issue Nov 27, 2020 that may be closed by this pull request
Copy link
Contributor Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Added the changelog entry.

src/watchdog/observers/kqueue.py Show resolved Hide resolved
Copy link
Contributor Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Fixed the tests. They actually work now.

Comment on lines +21 to +26
if case_sensitive:
path = PurePosixPath(path)
else:
included_patterns = {pattern.lower() for pattern in included_patterns}
excluded_patterns = {pattern.lower() for pattern in excluded_patterns}
path = PureWindowsPath(path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the handling of case-sensitivity into _match_path so that the test for _match_path can use {"*.py"} and {"*.PY"} to raise the ValueError.

_match_path is still assuming that included_patterns and excluded_patterns are sets, which is guaranteed by filter_path and match_any_paths. I'm also not against moving the call to set() over here.

("/users/gorakhargosh/foobar.py", {"*.py"}, {"*.PY"}, True, True),
("/users/gorakhargosh/foobar.py", {"*.py"}, {"*.PY"}, True, True),
("/users/gorakhargosh/", {"*.py"}, {"*.txt"}, False, False),
("/users/gorakhargosh/foobar.py", {"*.py"}, {"*.PY"}, False, ValueError),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once I moved the if case_sensitive to _match_path, I was able to use plain old strings as input as well.

@bstaletic
Copy link
Contributor Author

Travis is done. The errors don't seem related to my changes...

  • A TypeError in eventlet - there on the latest master.
  • A -choco install python3 --version=3.7.9 failure - also there on master.
  • A supposed thread leak on Windows and Python 3.9 - I assume it's a flake?

@BoboTiG
Copy link
Collaborator

BoboTiG commented Nov 27, 2020

Yes, those are known issues.

@bstaletic
Copy link
Contributor Author

Is there anything else I should do? Travis has finished running, it's just the github status that is stuck.

@BoboTiG BoboTiG merged commit efdc69f into gorakhargosh:master Nov 28, 2020
@BoboTiG
Copy link
Collaborator

BoboTiG commented Nov 28, 2020

Thanks a lot @bstaletic 🍾

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.

Move to use pathlib instead of os.path and pathtools
2 participants