Skip to content

filecmp and tempfile can handle pathlib.Path - #2267

Closed
scivision wants to merge 6 commits into
python:masterfrom
scivision:master
Closed

filecmp and tempfile can handle pathlib.Path#2267
scivision wants to merge 6 commits into
python:masterfrom
scivision:master

Conversation

@scivision

@scivisionscivision commented Jun 20, 2018

Copy link
Copy Markdown
Contributor

filecmp can handle pathlib.Path, including:

  • filecmp.cmp
  • filecmp.cmpfiles
  • filecmp.dircmp

Before this patch, doing something like

frompathlibimportPathimportfilecmpfilecmp.dircmp(Path('/tmp'), '/tmp')

would work fine when executed, but would give mypy errors like

error: Value of type variable "AnyStr" of "dircmp" cannot be "Path"

After this change, type checking works as expected

@scivision

scivision commented Jun 20, 2018

Copy link
Copy Markdown
ContributorAuthor

Hmm, I think this may need to be split into stdlib/3.6/filecmp.pyi.

Do the maintainers concur?

The other option per https://github.com/python/typeshed/blob/master/CONTRIBUTING.md#stub-versioning would be to use if statements inside the existing filecmp.pyi.

@gvanrossum

Copy link
Copy Markdown
Member

I would use if sys.version_info >= (3, 6).

@scivisionscivision changed the title filecmp can handle pathlib.Pathfilecmp and tempfile can handle pathlib.PathJun 21, 2018
@scivisionscivision changed the title filecmp and tempfile can handle pathlib.Pathfilecmp and tempfile can handle pathlib.PathJun 21, 2018
@scivision

Copy link
Copy Markdown
ContributorAuthor

OK I believe the fixes are ready to go.

if sys.version_info >= (3, 6):
def cmp(f1: Union[bytes, Text, os.PathLike], f2: Union[bytes, Text, os.PathLike],
shallow: Union[int, bool] = ...) -> bool: ...
def cmpfiles(a: Union[AnyStr, os.PathLike], b: Union[AnyStr, os.PathLike], common: Iterable[Union[AnyStr, os.PathLike]],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because this uses AnyStr, we should ideally also use a type variable in the PathLike case (so PathLike[AnyStr]). But this sort of things tends to run into mypy bugs sometimes. We should check a few cases like these:

  • cmpfiles(bytes, bytes) returns Tuple[List[bytes], ...]
  • cmpfiles(str, str) returns Tuple[List[str], ...]
  • cmpfiles(PathLike[str], PathLike[str]) returns Tuple[List[str], ...]
  • cmpfiles(PathLike[str], bytes) is an error

You can check these in mypy by using reveal_type.

hide: Optional[Sequence[AnyStr]] = ...) -> None: ...

if sys.version_info >= (3, 6):
def __init__(self, a: Union[AnyStr, os.PathLike], b: Union[AnyStr, os.PathLike],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, we should use PathLike[AnyStr] but should verify it actually works.

Also, do the ignore and hide parameters also accept paths?

Comment threadstdlib/3/tempfile.pyi
def gettempprefixb() -> bytes: ...

if sys.version_info >= (3, 6):
def TemporaryFile(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Most of these functions have the same issue with AnyStr.

@srittau

Copy link
Copy Markdown
Collaborator

@scivision Are you still interested in this pull request?

@scivision

Copy link
Copy Markdown
ContributorAuthor

Hi @srittau it seems like the fix grew bigger. I don't have much time lately so I have just been using type: ignore

@srittau

Copy link
Copy Markdown
Collaborator

Thank you for your work on this. I am closing this PR for now. Please reopen it or open a new PR if you wish to finish it.

Sign up for freeto 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.

4 participants

@scivision@gvanrossum@srittau@JelleZijlstra