Skip to content

Conversation

@adamjstewart
Copy link

First time contributor, honestly not sure if I'm in the right place.

fnmatch is a module for "Unix filename pattern matching". However, at the moment, the type hints do not support pathlib paths. From my testing, pathlib paths work just fine.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

First time contributor, honestly not sure if I'm in the right place.

Welcome! I think you're in the right place :-)

From my testing, pathlib paths work just fine.

Hmm, it looks to me like this is because fnmatch uses os.path functions internally to normalize names, and os.path functions will all happily accept os.PathLike objects as well as strings. (pathlib.Path objects conform to the os.PathLike protocol.) The fact that the docs talk about "names" throughout rather than "paths" implies pretty strongly to me that the intended purpose of the module is to work with strings -- so I'm not sure this is a good idea (I'd be interested in hearing what other maintainers think). The risk is that if fnmatch is refactored at some point and starts using some other technique to normalize arguments passed in, support for os.PathLike objects might accidentally be broken, since support for os.PathLike objects has never been documented or promised as far as I can see).

adamjstewart and others added 2 commits August 13, 2024 15:31
@AlexWaygood
Copy link
Member

(You'll need to add from _typeshed import GenericPath to the top of the file)

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

def fnmatchcase(name: AnyStr, pat: AnyStr) -> bool: ...
def filter(names: Iterable[AnyStr], pat: AnyStr) -> list[AnyStr]: ...
def fnmatch(name: GenericPath[AnyStr], pat: AnyStr) -> bool: ...
def fnmatchcase(name: GenericPath[AnyStr], pat: AnyStr) -> bool: ...
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work

>>> import fnmatch
>>> from pathlib import Path
>>> fnmatch.fnmatchcase(Path("hi"), "x")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jelle/Library/Application Support/uv/python/cpython-3.8.19-macos-aarch64-none/lib/python3.8/fnmatch.py", line 71, in fnmatchcase
    return match(name) is not None
TypeError: expected string or bytes-like object

The other functions do currently work with paths, but I'm not sure we should change the stubs; it feels accidental, and the module documentation talks about "path strings".

Copy link
Collaborator

@hauntsaninja hauntsaninja Aug 22, 2024

Choose a reason for hiding this comment

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

Another reason to not accept Paths: pathlib's trailing slash normalisation can also be undesirable in fnmatch like situations, e.g. Black has had bugs because of this kind of thing

@srittau
Copy link
Collaborator

srittau commented Aug 22, 2024

Thanks for your contribution, @adamjstewart! Based on the comments by my co-maintainers, I'm going to close this PR, though. Being able to use paths instead of strings looks like an unintended side effect of the implementation and could introduce subtle bugs.

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.

6 participants