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

tidy ResilientSession implementation #1366

Merged
merged 27 commits into from
Jun 11, 2022

Conversation

adehad
Copy link
Collaborator

@adehad adehad commented Apr 23, 2022

Summary of Changes

  • Resilient Session code cleanup, removing dead code and other code repetition
  • Updated workaround for retrying the MultipartEncoder scheme for attachments

TODO

  • Probably broke some tests that need to be fixed
  • Make sure docstrings are ok
  • add some tests for the new retry object

Special Thanks:

@datashaman : https://stackoverflow.com/questions/23267409/how-to-implement-retry-mechanism-into-python-requests-library/23271433#comment58969710_23271433

Co-Authored-By: datashaman <59514+datashaman@users.noreply.github.com>

@adehad adehad added the feature label Apr 23, 2022
@adehad adehad force-pushed the feature/new_resilient_session_implementation branch 5 times, most recently from ccc921b to febf197 Compare April 23, 2022 18:45
jira/client.py Outdated Show resolved Hide resolved
@adehad adehad requested a review from studioj April 23, 2022 18:52
@adehad adehad force-pushed the feature/new_resilient_session_implementation branch from febf197 to 6985a6f Compare May 1, 2022 02:26
@adehad
Copy link
Collaborator Author

adehad commented May 1, 2022

@StefanoBoriero , could I bother you to have a look at this?

As you were recently in this code I thought you might be able to critique this !

jira/resilientsession.py Outdated Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
@adehad adehad marked this pull request as ready for review May 1, 2022 10:59
@adehad adehad temporarily deployed to cloud May 1, 2022 11:10 Inactive
@StefanoBoriero
Copy link
Contributor

@StefanoBoriero , could I bother you to have a look at this?

As you were recently in this code I thought you might be able to critique this

With pleasure! I'll find some time in the next few days to do so

Copy link
Contributor

@StefanoBoriero StefanoBoriero left a comment

Choose a reason for hiding this comment

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

Good improvements! I left some comments mostly about refactoring, let me know what you think

jira/resilientsession.py Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
jira/resilientsession.py Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
jira/resilientsession.py Show resolved Hide resolved
@adehad adehad force-pushed the feature/new_resilient_session_implementation branch from b4ed5dc to a33aceb Compare May 3, 2022 23:26
@adehad
Copy link
Collaborator Author

adehad commented May 3, 2022

Thanks @StefanoBoriero, I've tried to address most of the concerns, with some discussions points.

I've organised my commits to tackle individual comments (as GitHub a bit weird this way) and left it up to you to resolve threads that I've addressed.

Copy link
Collaborator

@studioj studioj left a comment

Choose a reason for hiding this comment

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

quite a lengthy PR ... I'd like to test this out before giving my "official" approval :) I'll try to do so this week

jira/client.py Outdated Show resolved Hide resolved
jira/client.py Show resolved Hide resolved
jira/resilientsession.py Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
@adehad adehad temporarily deployed to cloud May 7, 2022 13:57 Inactive
jira/resilientsession.py Outdated Show resolved Hide resolved
studioj
studioj previously approved these changes May 25, 2022
Copy link
Collaborator

@studioj studioj left a comment

Choose a reason for hiding this comment

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

rework looks ok and the code is covered in the functional tests ... only the extra doc still todo

Copy link
Collaborator Author

@adehad adehad left a comment

Choose a reason for hiding this comment

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

Thanks @studioj , I'll make some time to finish the PR

jira/utils/__init__.py Outdated Show resolved Hide resolved
@adehad adehad force-pushed the feature/new_resilient_session_implementation branch from e8fb1c7 to 57c6357 Compare June 5, 2022 23:44
@adehad adehad force-pushed the feature/new_resilient_session_implementation branch from 57c6357 to 3c217dd Compare June 5, 2022 23:47
@adehad adehad temporarily deployed to cloud June 6, 2022 00:03 Inactive
jira/client.py Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
jira/resilientsession.py Show resolved Hide resolved
jira/resilientsession.py Outdated Show resolved Hide resolved
@adehad adehad requested a review from studioj June 6, 2022 22:04
@adehad adehad temporarily deployed to cloud June 6, 2022 22:17 Inactive
@adehad adehad merged commit c6d59a1 into main Jun 11, 2022
@adehad adehad deleted the feature/new_resilient_session_implementation branch June 11, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants