-
-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move all public methods to the api namespace #19
Conversation
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Welcome to Codecov 🎉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 ☂️ |
v1.0.0 moved the public api to propcache.api see aio-libs/propcache#21 see aio-libs/propcache#18 see aio-libs/propcache#19
v1.0.0 moved the public api to propcache.api see aio-libs/propcache#21 see aio-libs/propcache#18 see aio-libs/propcache#19
v1.0.0 moved the public api to propcache.api see aio-libs/propcache#21 see aio-libs/propcache#18 see aio-libs/propcache#19
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...). |
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 |
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. |
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 |
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 |
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. |
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 |
I'll hold on doing anything until webknjaz has a chance to respond. |
@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 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 As for keeping some imports in place, even temporarily, 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 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. |
But, the response there was to explicitly allow imports in libraries:
I guess I'm not really seeing how it's any different if everything is imported into api.py instead of |
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. |
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
andunder_cached_property
methods. They have moved topropcache.api.cached_property
andpropcache.api.under_cached_property
respectively.Related issue number
#18