Aiohttp and with_bind method not working together

[Reskov]

• GINO version: 0.8.3
• Python version: 3.7.4
• asyncpg version:
• aiocontextvars version:
• PostgreSQL version:

Description

with_bind does not work anymore with aiohttp extension after commit 5e5eabb

with_bind are passing loop as an positional argument, but positional argument was removed in gino 0.8.3, and only for aiohttp engine:(

Is this expected?

What I Did

async def a():
    from gino.ext.aiohttp import Gino
    gino = Gino()
    async with gino.with_bind("postgresql://user:pass@localhost:5432/test"):
        pass

await a()

  File "<ipython-input-2-78a668b1753a>", line 4, in a
    async with gino.with_bind("postgresql://user:pass@localhost:5432/test"):
  File "/venv/lib/python3.7/site-packages/gino/api.py", line 181, in __aenter__
    return await api.set_bind(bind, loop, **kwargs)
TypeError: set_bind() takes 2 positional arguments but 3 were given

[wwwjfy]

Hi, sorry for late reply.
Yes, it's expected. The change of loop deprecated is actually from aiohttp: aio-libs/aiohttp#3374

[Reskov]

To my mind it was not necessary to change signature of the method inside aiohttp subclass,
which made it incompatible with some of the base class methods (like with_bind).

Currently Gino base class has signature for set_bind method:

async def set_bind(self, bind, loop=None, **kwargs):

but ONLY aiohttp subclass have the other one

async def set_bind(self, bind, **kwargs):

Take a look pls on https://github.com/fantix/gino/blob/master/gino/api.py#L180, BindContext are using loop as an positional argument, but unfortunately it is incompatible with new aiohttp set_bind anymore.

So I suggest to revert only set_bind signature change. I'm still wondering why signature was changed, loop was an optional argument.

async def set_bind(self, bind, loop=None, **kwargs):

One more point, now it violates with one of the base of the OOP principle: Liskov substitution

[wwwjfy]

Generally, yes, we should keep the signatures same for parent and child classes. But in practice, it's not possible to follow all the principles.
aiohttp is special because of the upstream change. For the extension, I'd also like to follow the convention defined by the framework or library. And I don't want to surprise users if they find the loop passed in doesn't work, in a subtle way, so I choose the break-early way.
A less aggressive way might be adding deprecation warnings like aiohttp does, but as Gino is a relatively new framework, I feel it's the privilege we have now. Because it's before 1.0, somehow we expect things to be not backward compatible.

[Reskov]

Oh, that's very disappointing... I guess I'm stupid, but may you explain in details what was the problem with previous set_bind code?

I see that in aiohttp app loop is deprecated, can we just not to send it to set_bind and keep signature unchanged?

 async def set_bind(self, bind, loop=None, **kwargs):
        kwargs.setdefault('strategy', 'aiohttp')
        return await super().set_bind(bind, loop=loop, **kwargs)

Why removing optional loop parameter is so necessary? At first glance it does nothing except with_bind method breaking :)

P.S. I'm using with_bind on my http server management commands.

[wwwjfy]

Probably should do kwargs.popup('loop') since it's deprecated 🤔
By not send it to set_bind, do you mean to accept loop but not use it in super().set_bind()? If yes, I see it'll confuse users in case there is actually a separate loop prepared for this but not being used.

Signature change is definitely not ideal, but to me it's the better choice here.

If I misunderstand, let me know what you actually mean. :)

I feel it's a dilemma maintaining extensions as they're not versioned. When the dependency makes breaking changes, though it's good to keep up, it may also break existing code. Maybe a better choice is to maintain the extension in another (versioned) library, or even in the project itself.

[wwwjfy]

Closing due to inactivity. Feel free to reopen.

[dparker2]

Very odd to see that making with_bind completely unusable in the aiohttp extension is intended.

[Reskov]

what do @fantix think about this? Is it normal that the with_bind method does not work for the aiohttp extension?

[wwwjfy]

I don't quite remember what happened but I think I misunderstood. Sorry for that.
My intention was to not pass loop to aiohttp, but not disable with_bind usage in aiohttp extension.
Let me try to reproduce the warning and see if it's fine to use loop to make with_bind work.

[wwwjfy]

@Reskov you're right. I think we should add loop back to set_bind. I didn't think of with_bind and pay enough attention to your comments.
I'll add it back to 0.8.x and new ext repo. If there is no more backports to be done, will also release a new version in a few days.

Sorry for inconvenience.