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

refactor: make all services context managers #242

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

doronz88
Copy link
Owner

@doronz88 doronz88 commented Mar 1, 2022

No description provided.

@lgtm-com
Copy link

lgtm-com bot commented Mar 1, 2022

This pull request introduces 1 alert when merging dcd53d1 into 05c93cd - view on LGTM.com

new alerts:

  • 1 for Multiple calls to `__init__` during object initialization

@doronz88 doronz88 force-pushed the refactor/service_context_managers branch from dcd53d1 to 8965059 Compare March 1, 2022 14:43
@lgtm-com
Copy link

lgtm-com bot commented Mar 1, 2022

This pull request introduces 1 alert when merging 8965059 into 05c93cd - view on LGTM.com

new alerts:

  • 1 for Multiple calls to `__init__` during object initialization

@vToMy
Copy link

vToMy commented Mar 1, 2022

I think it will be better to open the connection on entering instead in the constructor.
And having some kind of open/start method for every close.

@doronz88
Copy link
Owner Author

doronz88 commented Mar 1, 2022

@vToMy you are definetely correct but such a change will both:

  • break all previous api uses - if we'll make such a change it will be in v2.0.0.
  • we still want our users to have the option to use the api without worrying for resource management if they don't explicity want to.

@doronz88 doronz88 force-pushed the refactor/service_context_managers branch from 8965059 to 9837f45 Compare March 1, 2022 16:15
@lgtm-com
Copy link

lgtm-com bot commented Mar 1, 2022

This pull request introduces 1 alert when merging 9837f45 into 05c93cd - view on LGTM.com

new alerts:

  • 1 for Unused import

@doronz88 doronz88 force-pushed the refactor/service_context_managers branch from 9837f45 to ee89638 Compare March 1, 2022 16:32
@doronz88 doronz88 requested a review from matan1008 March 1, 2022 16:35
@vToMy
Copy link

vToMy commented Mar 1, 2022

For backward compatability it's possible to have some kind of ctor flag (similar to autopair in lockdown)

@doronz88 doronz88 force-pushed the refactor/service_context_managers branch 4 times, most recently from 24dcf54 to 9b84c21 Compare March 2, 2022 11:27
@lgtm-com
Copy link

lgtm-com bot commented Mar 2, 2022

This pull request introduces 1 alert when merging 9b84c21 into 05c93cd - view on LGTM.com

new alerts:

  • 1 for Unused import

@doronz88 doronz88 force-pushed the refactor/service_context_managers branch from 9b84c21 to 022badc Compare March 2, 2022 11:42
@doronz88 doronz88 merged commit cec0158 into master Mar 2, 2022
@doronz88 doronz88 deleted the refactor/service_context_managers branch March 2, 2022 22:29
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