Discourage inheritance from some aiohttp classes

[asvetlov]

In first line I see web.Application and ClientSession.
If user want to extend functionality -- I'd like to encourage aggregation, not inheritance.

It could be done easy by adding __subclass_init__ thunder method, the method should generate a warning (UserWarning maybe).
The method is not called by Python 3.5 (but not generates a error too), on Python 3.6+ everything should work fine. Who cares about Python 3.5?

[HiImJulien]

Why would you prefer aggregation over inheritance in the special case of web.Application?

It seems to be a viable design decision, when creating simple applications. It helps to structure it.

[asvetlov]

It seems not.

1. Inheritance from Application pollutes both namespace: it adds user names for base application, application attributes from future versions of aiohttp may clash with user ones.
2. web.Application is not designed as base class with overriding virtual methods in children. All inversed control flows are intentionally implemented as signals and registered callbacks, not overridden methods.
3. Nothing prevents building a new custom class with web.Application attribute, it makes roles and responsibilities clear and straightforward.

[eigenein]

Nothing prevents building a new custom class with web.Application attribute, it makes roles and responsibilities clear and straightforward.

How would you access the aggregating object from request.app and from within a template app objects?

[asvetlov]

You can push the reference to your aggregating instance into the application as app['MY_KEY'] = aggregator, isn't it?

[eigenein]

I guessed that you will propose that solution, but of course I wanna argue. :)

So looking at it we have two objects referencing each other, just because inheritance is discouraged:

• assert foo.app == app
• assert app['foo'] = foo
• assert request.app['foo'] is foo

Which is a viable solution of course, but don't you find it to be a not-so-good OOP design?

Not to say, it breaks static type checking because tools don't know what is app['foo'] whereas subclassing does allow annotating a custom attribute.

[asvetlov]

Software design is always a compromise.
Honestly, I never need to use an aggregator but push all needed properties into application's dictionary.
For type checking you can use a little helper:

def foo(app: Application) -> FooType:
    return cast(FooType, app['FOO_LABEL'])

I understand that the solution is not ideal.
Maybe we need to come up with a better solution but it should be not an inheritance.

Suppose you have something like

class MyApp(Application):
    def foo(self):
        return something

Eventually, in aiohttp we decided to add a foo attribute to the base class. As the result, code is broken in a hard way. Depending on how did you add your foo and how foo is added to base application one implementation wins. Which one is a complex question: instance attribute overrides a class method, but if a class declared a property the property setter is called and so on.

Replacing string keys for application storage with a ContextVar-styled object is an interesting idea.
It can solve the static typing problem and avoid names clashing. I did not experiment with it yet though.

[aparamon]

@asvetlov
In tests we use rewriting of netloc to always access tested service by canonical address, regardless if test is run against localhost or remote service:

class AsyncTestSession(aiohttp.ClientSession):
    """
    Rewrites request netloc (host:port) and prepends base_path.
    """

    def __init__(self, netloc, base_path='', *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.netloc = netloc
        self.base_path = base_path

    def _request(self, method, url, *args, **kwargs):
        scheme, netloc, path, params, query, fragment = urllib.parse.urlparse(url)
        if path:
            path = '/'.join((self.base_path, path.lstrip('/')))
        url = urllib.parse.urlunparse((scheme, self.netloc, path, params, query, fragment))
        return super()._request(method, url, *args, **kwargs)

We store netloc and base_path attributes for logging and debugging. What is considered the appropriate solution to avoid DeprecationWarning and future breakage?