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

[feature] Add support for requests timeout #1574

Merged
merged 2 commits into from
Feb 10, 2023
Merged

[feature] Add support for requests timeout #1574

merged 2 commits into from
Feb 10, 2023

Conversation

Victor-D
Copy link
Contributor

@Victor-D Victor-D commented Dec 7, 2022

This PR adds the timeout parameter when performing HTTP request and fix some comments/typing related to the timeout parameter.

Jira is based on the python requests library to perform HTTP requests so we can use its timeout parameter as explained here : https://requests.readthedocs.io/en/latest/user/advanced/#timeouts

I think this library planed to do so, but it was not implemented.

The timeout parameter still has a None default value so no change is expected on applications which don't set a timeout value. For other applications who though they were using it then it will start to work.

@Victor-D Victor-D changed the title Add support for requests timeout [feature] Add support for requests timeout Dec 7, 2022
bboe
bboe previously approved these changes Feb 9, 2023
Copy link

@bboe bboe left a comment

Choose a reason for hiding this comment

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

I look forward to seeing this PR merged since the functionality was accidentally lost here:
https://github.com/pycontribs/jira/pull/1366/files#diff-0605333df4b31ca7f97b2591e3bcba9c71ea201f4a88e383316e7f6c1c478e06L177

studioj
studioj previously approved these changes Feb 9, 2023
@studioj
Copy link
Collaborator

studioj commented Feb 9, 2023

Looks cool and thank you alot for adding a few tests. The only issue I foresee here @Victor-D is that this "bug-fix" might break some systems which have defined a super low timeout which unwittingly worked.
what's your opinion @adehad
@Victor-D could you rebase and solve the conflicts. That would be awesome

@adehad
Copy link
Contributor

adehad commented Feb 10, 2023

I didn't realise this was a regression, we should merge this in that case. We should be able to debug any issues for timeout errors if they come up in issues or discussions so let's try it for now

@adehad adehad dismissed stale reviews from studioj and bboe via 4082f34 February 10, 2023 10:05
@adehad adehad merged commit 3b5b7b4 into pycontribs:main Feb 10, 2023
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.

4 participants