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

Fix Context.cd() behavior to match the type hint #922

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rouge8
Copy link

@rouge8 rouge8 commented Feb 27, 2023

Issue found when fixing the type hint in typeshed: python/typeshed#9823 (review)

Issue found when fixing the type hint in typeshed:
python/typeshed#9823 (review)
@rouge8
Copy link
Author

rouge8 commented Feb 27, 2023

Ahh I see, reading the docstring it's the typehint that is wrong. Will fix.

@rouge8
Copy link
Author

rouge8 commented Feb 27, 2023

Hmm, so the way I see to fix this would be to use a typing.Protocol with a __str__ method, but it looks like invoke supports Python 3.6 which doesn't have that and doesn't currently have a dependency on typing_extensions. Should I add a dependency on typing_extensions to the dev requirements?

@JelleZijlstra
Copy link

All objects have __str__ so a protocol for __str__ doesn't make sense. If you only want to support objects that are actually meant to be paths, os.fspath is a better choice.

@kuwv
Copy link
Contributor

kuwv commented Feb 27, 2023

I should have checked typeshed. I knew there were types for paramiko but no idea one was created for invoke. I'll see what I can reuse here.

Thanks for the assist!

@kuwv
Copy link
Contributor

kuwv commented Jul 10, 2023

#956 The os.fspath requires an object to have __fspath__ instead of __str__ for the tests to pass.

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.

3 participants