Improve the handling of stringified annotations in _takes_container

[guacs]

Summary

Pull Request Check List

• Typos aside (please, always submit typo fixes!), I understand that this pull request may be closed in case there was no previous discussion.
• Do not open pull requests from your main branch – use a separate branch!
  • There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
  • This is not a pre-requisite for your your pull request to be accepted, but you have been warned.
• Added tests for changed code.
  • The CI fails with less than 100% coverage.
• New APIs are added to our typing tests at https://github.com/hynek/svcs/blob/main/tests/typing/.
• Updated documentation for changed code.
  • New functions/classes have to be added to docs/core-concepts.md or one of the integration guides by hand.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
    • The next version is the second number in the current release + 1. The first number represents the current year. So if the current version on PyPI is 23.1.0, the next version is gonna be 23.2.0. If the next version is the first in the new year, it'll be 24.1.0.
• Documentation in .rst and .md files is written using semantic newlines.
• Changes (and possible deprecations) are documented in the changelog.
• Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

This should work in all the cases that are included in the tests, but it will still fail in the following case:

from __future__ import annotations

from svcs import Container as ServiceContainer

def get_service(container: ServiceContainer) -> None:
    ...

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 the locals of inspect.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.

[hynek]

OK 3.8 & 3.9 are failing again – but you're not even using get_type_hints? :|

[guacs]

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.

[hynek]

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 use get_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.

[guacs]

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 use get_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.

[hynek]

Compromise: dumb string compare on Legacy Pythons, your new code on Good Pythons – what do you say?

[guacs]

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?

[hynek]

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. 🤔

[hynek]

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"})).

[guacs]

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"})).

This is really nice. Okay then here's another idea. What if you place the following constraints:

• factories can only have one positional argument at most and if there is one, then it must be the container
• all other arguments must be keywords

[hynek]

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?

[guacs]

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.

[hynek]

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%.

[guacs]

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.

[hynek]

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?

[guacs]

My suggestion is the following:

• use the implementation of _takes_container in this PR for Python 3.10+ and use the old implementation for older versions (with an extra string comparison for "Container")
• document the different scenarios in which the user will face issues
• recommend the usage of svcs_container as the name of the argument for getting the container
• make the name of the argument to the container configurable so that users can use another name instead of svcs_container if they wish to do so

EDIT: 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.

[hynek]

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!

[guacs]

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.

[hynek]

Yeah I'll add something like that – don't worry about it and just fix what's breaking you. :)

[guacs]

@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.

[hynek]

Super cool, thanks!