Closed guacs closed 11 months ago
OK 3.8 & 3.9 are failing again – but you're not even using get_type_hints? :|
I should have read the documentation in more detail :P eval_str
was introduced only in 3.10 :/
Maybe instead of signature
, we should use get_type_hints
to get the annotations from the function.
OK 3.8 & 3.9 are failing again – but you're not even using get_type_hints? :|
I should have read the documentation in more detail :P
eval_str
was introduced only in 3.10 :/
FWIW, I'm OK if you write two versions hidden behind a huge if sys.version
block.
Maybe instead of
signature
, we should useget_type_hints
to get the annotations from the function.
Yes, but as you'll notice, it's a bit more complicated that it looks like. 🤓 Because I've tried cobbling something together and your PR was so fast I gave up on it.
OK 3.8 & 3.9 are failing again – but you're not even using get_type_hints? :|
I should have read the documentation in more detail :P
eval_str
was introduced only in 3.10 :/FWIW, I'm OK if you write two versions hidden behind a huge
if sys.version
block.Maybe instead of
signature
, we should useget_type_hints
to get the annotations from the function.Yes, but as you'll notice, it's a bit more complicated that it looks like. 🤓 Because I've tried cobbling something together and your PR was so fast I gave up on it.
Yeah, this is quite frustrating actually. If the factory functions are annotated, then we can use the get_type_hints
route, but if they aren't then we have to go through the inspect.signature
route to handle cases where the first argument is name svcs_container
.
Compromise: dumb string compare on Legacy Pythons, your new code on Good Pythons – what do you say?
I have a doubt. The factories are only supposed to take zero or one argument and if it takes in an argument, it has to be the container correct?
If this is the case, can't we just return len(sig.parameters) == 1
instead of having to deal with all this typing related stuff?
hmmm the reason I'm strict with this is that I want to keep the option of adding more arguments down the road.
It correct though that it is a bit stupid to error out on non-0/1 and just ignore the one if it has the wrong name/type.
Technically it should error out, not return False. 🤔
Like one example of what I maybe want to do in the future is allowing passing arguments to factories (x = svcs.get(X, foo="bar")
call's X's factory like factory(kwargs={"foo": "bar"})
).
Like one example of what I maybe want to do in the future is allowing passing arguments to factories (
x = svcs.get(X, foo="bar")
call's X's factory likefactory(kwargs={"foo": "bar"})
).
This is really nice. Okay then here's another idea. What if you place the following constraints:
container
Hmm, why would we enforce that tho? Like: what problem are we solving right now?
I find it nicer to say to users of at least 3 years old Pythons that their accuracy of container detection is a bit lower (for comparison: both attrs and data classes check for ClassVar using string compare to avoid importing typing) than enforcing new rules on everybody.
Or am I missing something?
Hmm, why would we enforce that tho? Like: what problem are we solving right now?
I find it nicer to say to users of at least 3 years old Pythons that their accuracy of container detection is a bit lower (for comparison: both attrs and data classes check for ClassVar using string compare to avoid importing typing) than enforcing new rules on everybody.
Or am I missing something?
The current way of comparing the strings wouldn't work if a user aliases Container
to something (like the example I have given in the description of the PR). With the constraint I proposed, there would be no issues related to typing nor any issues related to the aliasing etc.
Agreed, but I'm fine to say "no aliasing pre-3.10" and let everyone use factory(*, s: [svcs.]Container)
. I find that is a reasonable compromise – especially given that svcs is a young project and the cumulative 3.8+3.9 downloads are sub 3%.
Agreed, but I'm fine to say "no aliasing pre-3.10" and let everyone use
factory(*, s: [svcs.]Container)
. I find that is a reasonable compromise – especially given that svcs is a young project and the cumulative 3.8+3.9 downloads are sub 3%.
Yes, but unfortunately aliasing will not work if it's under a TYPE_CHECKING
block regardless of the Python version. For example:
from __future__ import annotations
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from svcs import Container as ServiceContainer
def get_service(container: ServiceContainer) -> Service:
...
The reason I feel it would be nice to support this scenario is because it's a common pattern now and linters like ruff
will move these types automatically into the TYPE_CHECKING
block even if the user places them outside it (of course, you can disable that).
Of course, you could just document all these edge cases/caveats and tell users to not alias within a TYPE_CHECKING
block. Adding the constraints I proposed would make things bit inflexible for change in the future without breaking changes, so it's completely reasonable to go with the approach of documenting these caveats.
Yeah TYPE_CHECKING comes with tons of caveats which is why I've stopped using it unless I have to for breaking import cycles. Eg autodoc also completely falls apart.
Since we're generally agreeing, what would you suggest concretely?
My suggestion is the following:
_takes_container
in this PR for Python 3.10+ and use the old implementation for older versions (with an extra string comparison for "Container")svcs_container
as the name of the argument for getting the containersvcs_container
if they wish to do soEDIT: I just realized that the aliasing, type hint related issues can all be avoided if the user uses svcs_container
as the name for the argument.
I agree with points 1 and 2 and I think that's the only ones relevant to this PR. If you want to do only the first point, that would be already above and beyond too!
I'll update the PR accordingly. I'll try to write the documentation, but if it's something you'd like to do yourself then just let me know :)
Also, regarding the 3rd point, maybe "recommend" isn't the right word. I felt it would be nice if the docs mentioned that this is a workaround in case they have some limitation they're facing with respect to the typing and the containers not being injected into their factory functions. I was facing this issue and it took me some time to figure out that the easiest workaround was to just name my parameter svcs_container.
Yeah I'll add something like that – don't worry about it and just fix what's breaking you. :)
@hynek I think it's ready for another review. The tests are passing for me on both 3.8 and 3.9 locally.
Also, we don't need to have different implementations based on the version since if there's some exception then it falls back to using only inspect.signature(factory)
without the use of eval_str
or anything.
Summary
Pull Request Check List
main
branch – use a separate branch!docs/core-concepts.md
or one of the integration guides by hand.versionadded
,versionchanged
, ordeprecated
directives..rst
and.md
files is written using semantic newlines.This should work in all the cases that are included in the tests, but it will still fail in the following case:
This can be handled, but we would have to add something like this to the
Registry
and then pass in that signature namespace mapping to_takes_container
which would then pass it to thelocals
ofinspect.signature
.Please let me know if there are any changes that needs to be made and I'd be happy to do so :)
NOTE: I haven't updated the changelog yet.