Skip to content

Conversation

@rouge8
Copy link
Contributor

@rouge8 rouge8 commented Feb 27, 2023

@rouge8 rouge8 requested a review from AlexWaygood February 27, 2023 17:50
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

It looks to me like the runtime implementation is incorrect. They're calling str() on the path argument here, rather than os.fspath. That means that the function will work with str and pathlib.Path objects, but it won't actually work with arbitrary os.PathLike objects, even if that's what the type hints in the upstream source code say. So maybe the type hint should actually be str | pathlib.Path rather than StrPath.

(Upstream should probably update their implementation so that it calls os.fspath() rather than str() on the path parameter. Then the implementation would match the type hint that they've provided.)

@github-actions

This comment has been minimized.

@rouge8
Copy link
Contributor Author

rouge8 commented Feb 27, 2023

It looks to me like the runtime implementation is incorrect. They're calling str() on the path argument here, rather than os.fspath. That means that the function will work with str and pathlib.Path objects, but it won't actually work with arbitrary os.PathLike objects, even if that's what the type hints in the upstream source code say. So maybe the type hint should actually be str | pathlib.Path rather than StrPath.

(Upstream should probably update their implementation so that it calls os.fspath() rather than str() on the path parameter. Then the implementation would match the type hint that they've provided.)

Ooh, good catch. I'll open a PR upstream to fix that.

@rouge8 rouge8 requested a review from AlexWaygood February 27, 2023 18:02
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@AlexWaygood AlexWaygood changed the title invoke: Update Context.cd() to accept an os.PathLike or a string invoke: Update Context.cd() to accept a pathlib.Path or a string Feb 27, 2023
@github-actions
Copy link
Contributor

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

@AlexWaygood AlexWaygood merged commit f53d073 into python:main Feb 27, 2023
@rouge8 rouge8 deleted the invoke-pathlike branch February 27, 2023 18:14
rouge8 added a commit to rouge8/invoke that referenced this pull request Feb 27, 2023
Issue found when fixing the type hint in typeshed:
python/typeshed#9823 (review)
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.

2 participants