-
Notifications
You must be signed in to change notification settings - Fork 421
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
Issue 1299 #1300
Issue 1299 #1300
Conversation
Thanks for the contribution! Before we can merge this, we need @kykrueger to sign the Salesforce Inc. Contributor License Agreement. |
I've signed the CLA |
Close and reopen PR to run CLA check again. |
@kykrueger thanks for putting this PR together! As a first time contributor we had to click a button to run all the checks here and those are done now. You are all set from the CLA perspective. If you look at the build results it looks like you have one black formatting issue and one by pi type issue to fix. In parallel, @jacalata could you give this a review? |
Alright, I'll run black over it once. Are you using the defaults or the --preview flag? |
Defaults |
To push this along I've added a check to see if exceptions are returned from the blocking request. This was already returned before this PR, but the type hinting didn't reveal the possibility. When making my changes, I fixed a pyright error where it had complained that Exceptions could be returned, but were not hinted. This broke the checks for mypy. Now the returned exceptions will be raised. Before a typeerror would have probably occurred, but now the proper exception will be raised. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks! I'd like to add some tests with it, but if I don't get round to that I'll merge it this week anyway.
Thanks! |
@jacalata , I adjusted the settings of my config for Black to match those of your tests. |
2f73619
to
5653a3e
Compare
is there any update on this PR? our workflows ran into the same error described by @kykrueger and we had to revert back to |
closes #1299
Fixes issue with bad state of async_response.
Fixes issue that timeouts are not triggered when set to longer than 60s.