Skip to content

Update typing hints of timeouts to allow passing floats #234

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

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

ossareh
Copy link
Contributor

@ossareh ossareh commented Aug 25, 2021

The type hint for execute_timeout are incorrect; the call is ultimately passed to asyncio.wait_for which supports passing:

  • int; interpreted as seconds
  • float; interpreted as <seconds>.<milliseconds>.
  • None; which results in blocking forever

This commit fixes the type hint and updates the docs to inform the reader what results from passing in None.

My main concern about this change is not being clear why it was chosen to specify seconds as though they're ints; it may be that this change now forever subscribes this API to being compatible with asyncio.wait_for - and it's not clear to me if that is your overall intent.

@leszekhanusz
Copy link
Collaborator

Well spotted, we could indeed accept floats.
For the linting issues, you should install the correct dev dependencies with pip install .[dev], then run make check

You may want to also modify the typing hints in websockets.py:

        connect_timeout: int = 10,                                                                                      
        close_timeout: int = 10,                                                                                        
        ack_timeout: int = 10,                                                                                          
        keep_alive_timeout: Optional[int] = None

@ossareh ossareh force-pushed the master branch 2 times, most recently from 2d79705 to 54c36e0 Compare August 25, 2021 19:39
@ossareh
Copy link
Contributor Author

ossareh commented Aug 25, 2021

@leszekhanusz I tracked down the missing type hint and have now pushed up a fix for it. I also removed my prior two comments because they added noise rather than signal.

I have since worked out how to get my venv up and running; running make check turns up a lot of unrelated complaints from within python itself - so I feel as though I don't have the venv "exactly" right just yet.

Copy link
Collaborator

@leszekhanusz leszekhanusz left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@leszekhanusz leszekhanusz changed the title fix(gql.client): update type to permit passing floats Update typing hints of timeouts to allow passing floats Aug 26, 2021
@leszekhanusz leszekhanusz merged commit ca4021d into graphql-python:master Aug 26, 2021
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