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: added types to exec & tc_properties_get_tc_host #561

Merged

Conversation

Dandiggas
Copy link
Contributor

#557 - trying to solve this issue by adding types.

Copy link

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

this LGTM overall, thanks @Dandiggas!

@errordeveloper
Copy link

@kiview could you please approve the workflows to run?

@alexanderankin
Copy link
Collaborator

We'll take a look

@alexanderankin
Copy link
Collaborator

yep so we support python 3.9 for now so cant use the pipe syntax for typing.Union - but the other type is indeed wrong so we could merge a pr that just fixes a single type, would probably prefer a PR that fixes types more comprehensively like #504 - if you want to provide feedback on that branch, that could help it along - it was only not merged because i was not confident in 100% of the choices made in there at the time.

ill leave this open in case you want to replace with from typing import Union

@Dandiggas
Copy link
Contributor Author

@alexanderankin Now supports python 3.9, please review when possible

@alexanderankin alexanderankin changed the title added types to exec & tc_properties_get_tc_host fix: added types to exec & tc_properties_get_tc_host May 17, 2024
@alexanderankin alexanderankin merged commit 9eabb79 into testcontainers:main May 17, 2024
7 of 8 checks passed
@alexanderankin
Copy link
Collaborator

thank you for the contribution! if you are interested in improving typing in this library are other types that I have started but did not feel confident enough to merge #504

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants