PacktPublishing / Building-Data-Science-Applications-with-FastAPI

Building Data Science Applications with FastAPI, Published by Packt
MIT License
306 stars 157 forks source link

Discussion: consider merging `with` statements #3

Open pnijhara opened 3 years ago

pnijhara commented 3 years ago

Although the with statements mentioned below are written in tests, they can be merged. I am not very much sure about this but we can have a discussion on it.

  1. In chapter9/chapter9_db_test.py:
    29 @pytest.fixture
    30 async def test_client():
    31    app.dependency_overrides[get_database] = get_test_database
    32    async with LifespanManager(app):
    33        async with httpx.AsyncClient(app=app, base_url="http://app.io") as test_client:
    34            yield test_client
    35
    36
    37 @pytest.fixture(autouse=True, scope="module")

    this can reframed as:

    29 @pytest.fixture
    30 async def test_client():
    31    app.dependency_overrides[get_database] = get_test_database
    32    async with LifespanManager(app), httpx.AsyncClient(app=app, base_url="http://app.io") as test_client:
    33        yield test_client
    34
    35
    36 @pytest.fixture(autouse=True, scope="module")

Similarly in chapter9/chapter9_app_post_test.py, chapter9/chapter9_app_test.py, chapter9/chapter9_db_test.py. There could be other collapsable with statements that could be merged that I haven't looked.

frankie567 commented 3 years ago

Hi @pnijhara!

Thank you for raising this point! Indeed, the syntax you propose is completely equivalent and working. However, I find the former more readable: for developers discovering the code of this fixture, it's more obvious that we are first opening a context for the lifespan events, then another one for the HTTP client.

Therefore, I think we can keep it like this; it's not harmful IMO.

pnijhara commented 3 years ago

Yeah, I agree with you it is more readable. However, for the sake of others, I request you to keep this discussion open. Maybe someone finds this worthy.