aio-libs / propcache

Fast property caching
Apache License 2.0
7 stars 1 forks source link

Move all public methods to the api namespace #19

Closed bdraco closed 2 weeks ago

bdraco commented 2 weeks ago

What do these changes do?

Move public methods to the api namespace.

Are there changes in behavior for the user?

The top level module no longer provides the cached_property and under_cached_property methods. They have moved to propcache.api.cached_property and propcache.api.under_cached_property respectively.

Related issue number

18

codecov[bot] commented 2 weeks ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

Dreamsorcerer commented 2 weeks ago

Is there prior art for this approach? Personally, my first thought seeing something like this, is that's it's not pythonic and just adds verbosity to imports (which then makes me start to think of Java imports...).

bdraco commented 2 weeks ago

I had initially balked a bit to this approach but I’ve also been in a situation where you end up forced to have code loaded that isn’t needed anymore to maintain compatibility because it’s imported into __init__.py so I can see why it would be advantageous to avoid that problem in the future

Dreamsorcerer commented 2 weeks ago

I’ve also been in a situation where you end up forced to have code loaded that isn’t needed anymore to maintain compatibility because it’s imported into __init__.py

Arguably, you could use getattr for that (e.g. how gunicorn workers are imported in aiohttp.web). But, this looks like a single module library, so doesn't look like it'd benefit anyway. And for something like aiohttp, you'd decrease the amount of code loaded into memory, but sacrifice usability when every user now needs 20 lines of aiohttp imports in each of their modules.

bdraco commented 2 weeks ago

I agree it’s very unlikely to be a problem in the future, we could revert the change since 1.0.0 failed to ship

bdraco commented 2 weeks ago

I looked at what the change will be downstream to Home Assistant, and they’ll be ~86 new line changes to add .api which I agree isn’t great

Dreamsorcerer commented 2 weeks ago

I don't mind too much, it doesn't make much difference for this library. Mainly just wanted to know @webknjaz's arguments before this becomes accepted as aio-libs standard practice, as I'm struggling to see any benefits. Whether you want to revert or not, |I'll leave with you.

bdraco commented 2 weeks ago

Since 1.0.0 failed to upload to pypi anyways, I pulled it down from GitHub while we decide this so no downstream is affected

bdraco commented 2 weeks ago

I don't mind too much, it doesn't make much difference for this library. Mainly just wanted to know @webknjaz's arguments before this becomes accepted as aio-libs standard practice, as I'm struggling to see any benefits. Whether you want to revert or not, |I'll leave with you.

I'll hold on doing anything until webknjaz has a chance to respond.

webknjaz commented 2 weeks ago

@Dreamsorcerer one place this is coming from WPS412 that I know you asked about in the past. The documented rationale there is incomplete, though.

It's more or less obvious that if you import a.b but a also imports c internally, you'll get a side effect of more things loaded than you expected. This is a simple case but as projects grow, I observed a lot of human error happening, leading to unobvious import loops (we have tests in aiohttp to fight this, of course, but does this mean that every project has to implement a test scanning the disk for Python modules and attempting to import them in isolation to see if something blows up? Probably not).

I'm seeing cases where projects cannot use regular module-level imports and stick them into functions that are called later because too many things end up being loaded and initialized on import depending on other imports to happen first to the point of being both ridiculous and really difficult to fix.

That's outside aio-libs, here we tend to have more robust structure, but that's at a cost of higher cognitive load. Implementing a good layout is something that may reduce said load and human factor-caused problems, virtually for free. I don't mind that the callers might need to type in 4 more bytes in their imports.

One case that came to my mind within aiohttp, for example, was emitting deprecations and using an in-project class for that, which effectively made it impossible to suppress because referencing the exception in the warning suppression settings meant that in needed to be imported and the import itself was causing the deprecation warning. It's a very real problem that we need to be conscious about.

The libs still need to expose some high-level API, it just doesn't have to be in a way that would cause such side effects. And I figured that api.py might be such a place.

As for keeping some imports in place, even temporarily, __getattr__() is a good solution, it could handle deprecations too, if needed.

I think this is a good layout to have but of course, we're not going to break existing APIs. Still, for new projects, I believe this is what they should be modeled after. You mentioned aiohttp probably thinking that I'd suggest changing it too (I'm guessing). But no, aiohttp already has some of such namespacing through such proxy entry points (like aiohttp.web). The only thing to mention is that maybe we need to pay more attention to such entry point modules regardless of what they are called (.api seems like a good name for smaller libs and could be universal, unlike bigger frameworks). And perhaps move more things under __getattr__() in some cases.

I think I saw a blog post or something on this years ago but can't find it. But the approach is good, and Anthony agrees that the linting rule is useful. I've been practicing this where I can for some time, and I'm pretty happy about what it brings to the table.

Dreamsorcerer commented 2 weeks ago

@Dreamsorcerer one place this is coming from WPS412 that I know you asked about in the past. The documented rationale there is incomplete, though.

But, the response there was to explicitly allow imports in libraries:

And with libraries it is totally fine to tweak imports. Libraries are going to use --i-dont-controll-code anyway.

The libs still need to expose some high-level API, it just doesn't have to be in a way that would cause such side effects. And I figured that api.py might be such a place.

I guess I'm not really seeing how it's any different if everything is imported into api.py instead of __init__.py... The traceback for the mypy issue above doesn't even have __init__.py anywhere in it, the circular import doesn't seem like it'd be impacted by following this rule.

webknjaz commented 2 weeks ago

I guess I'm not really seeing how it's any different if everything is imported into api.py instead of __init__.py...

You're right. This aspect is not very different. But I think that it's still a good layout, since it enables us to expose things that are used more rarely in a more granular way.