falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

docs(examples/asgilook): update aioredis version to 2.0 #1987

Closed the-bets closed 2 years ago

the-bets commented 2 years ago

Summary of Changes

Update aioredis to version 2.0, the impact of which is found in the asgilook examples. Apply necessary code and API changes as well as update asgilook tutorial.

Both tools/mintest.sh and tox --recreate -e asgilook pass with these updates. Docs (for the tutorial) were also successfully built and changes correctly shown.

Related Issues

fixes #1938

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

PR template inspired by the attrs project.

codecov[bot] commented 2 years ago

Codecov Report

Merging #1987 (374571c) into master (9786798) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1987   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         6676      6676           
  Branches      1239      1239           
=========================================
  Hits          6676      6676           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9786798...374571c. Read the comment docs.

CaselIT commented 2 years ago

But could we maybe invent a use case for async process_startup (or alternatively process_shutdown)?

Maybe we could ping the server in process_startup using await self._redis.ping() and close the pool in process_shutdown by doing await self._redis.connection_pool.disconnect()?

vytas7 commented 2 years ago

Yeah that sounds great @CaselIT ! That makes sense because as I understand aioredis.from_url(...) just instantiates an object, you can provide any URL there, even if the referenced server is down or broken.

I think process_startup is enough though :sweat_smile:

the-bets commented 2 years ago

Sounds good! Thanks for the review, we'll plan on adding in that process_startup/process_shutdown example this week.

vytas7 commented 2 years ago

Hi again @the-bets , just checking if you are going to have an opportunity to work on that process_startup example? Otherwise I'm thinking we could add this ourselves in a followup PR, as your changeset seems to be really well polished as-is, and maybe we don't need to delay merging just because of the lack of that example.

the-bets commented 2 years ago

Hi again @the-bets , just checking if you are going to have an opportunity to work on that process_startup example?

Otherwise I'm thinking we could add this ourselves in a followup PR, as your changeset seems to be really well polished as-is, and maybe we don't need to delay merging just because of the lack of that example.

Hello again! Thanks for checking in. Mihai and I both got busy with work and life since the last time we talked on this PR. Neither of us have gotten to writing that example yet, and with this being a travel/holiday week for 1/2 of us, we probably won't get to it soon. We do not mind this PR being merged as-is for the time being.

If you want to add the example yourselves, that's also fine with us! We are still down to add it, but again likely won't get to it this week.

vytas7 commented 2 years ago

Thanks for the update! It's not that urgent, so if you're still committed to it, we can keep this open until we are approaching a release.

the-bets commented 2 years ago

Thank you @vytas7 and @CaselIT for the input, patience, and quick merge!