Skip to content

Conversation

@scivision
Copy link
Contributor

@scivisionscivision commented Jun 20, 2018

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
Copy link
ContributorAuthor

scivision commented Jun 20, 2018

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
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
ContributorAuthor

OK I believe the fixes are ready to go.

ifsys.version_info>= (3, 6):
defcmp(f1: Union[bytes, Text, os.PathLike], f2: Union[bytes, Text, os.PathLike],
shallow: Union[int, bool] = ...) ->bool: ...
defcmpfiles(a: Union[AnyStr, os.PathLike], b: Union[AnyStr, os.PathLike], common: Iterable[Union[AnyStr, os.PathLike]],
Copy link
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: ...

ifsys.version_info>= (3, 6):
def__init__(self, a: Union[AnyStr, os.PathLike], b: Union[AnyStr, os.PathLike],
Copy link
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?

defgettempprefixb() ->bytes: ...

ifsys.version_info>= (3, 6):
defTemporaryFile(
Copy link
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
Collaborator

@scivision Are you still interested in this pull request?

@scivision
Copy link
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
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