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

invoke: Update Context.cd() to accept a pathlib.Path or a string #9823

Merged
merged 3 commits into from
Feb 27, 2023

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