plone / guillotina

Python AsyncIO data API to manage billions of resources
https://guillotina.readthedocs.io/en/latest/
Other
187 stars 51 forks source link

Command: run the startup/shutdown and main logic inside the loop #1125

Closed masipcat closed 3 years ago

masipcat commented 3 years ago

The motivation behind this change is to fix https://github.com/plone/guillotina_elasticsearch/pull/86.

codecov-commenter commented 3 years ago

Codecov Report

Merging #1125 (d1890ea) into master (fecd945) will increase coverage by 0.1%. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1125     +/-   ##
========================================
+ Coverage    94.4%   94.4%   +0.1%     
========================================
  Files         372     372             
  Lines       31897   31897             
========================================
+ Hits        30093   30098      +5     
+ Misses       1804    1799      -5     
Impacted Files Coverage Δ
guillotina/contrib/pubsub/utility.py 87.0% <0.0%> (+2.2%) :arrow_up:
guillotina/contrib/redis/driver.py 86.5% <0.0%> (+2.3%) :arrow_up:
masipcat commented 3 years ago

Which is the reason to change to asyncio.run ? this does not allow to set the loop, we should change the api to not receive a loop as it can not be set on asyncio.run.

I didn't know we wanted to be able to change the loop and asyncio.run "just works". In fact, it takes care of some thing during the shutdown that we are not doing, and it seemed cleaner to use this function. I'm curious, which's the usecase to change the loop?

masipcat commented 3 years ago

Now I realize that maybe the PR title it's not accurate to the intention of this PR. The important change is that now it's "asyncio-first": all the startup, main, and shutodown logic runs inside the loop.

This can be accomplished as well without using asyncio.run, and I can change it if we need to be able to change the loop

bloodbare commented 3 years ago

Now I realize that maybe the PR title it's not accurate to the intention of this PR. The important change is that now it's "asyncio-first": all the startup, main, and shutodown logic runs inside the loop.

This can be accomplished as well without using asyncio.run, and I can change it if we need to be able to change the loop

The use case for reusing the loop its to overwrite Command class on a class that calls multiple commands or on tests environments. In any case the contract has loop as parameter and now its not used at any place.

bloodbare commented 3 years ago

Now I realize that maybe the PR title it's not accurate to the intention of this PR. The important change is that now it's "asyncio-first": all the startup, main, and shutodown logic runs inside the loop.

This can be accomplished as well without using asyncio.run, and I can change it if we need to be able to change the loop

I feel ok to have the main run as async, its a change on API which will breake some code, so it should not be a bugfix release, no ?

masipcat commented 3 years ago

I feel ok to have the main run as async, its a change on API which will breake some code, so it should not be a bugfix release, no ?

The PR is till backwards compatible with commands with non-async run functions. But if we feel this case can be removed, I can delete the code and change the minor number