Add support for Annotated types

[ljnsn]

Add support for Annotated types.

This is a relatively simple and straightforward implementation with minimal changes and seems to work fine.

Resolves #217

[davidparsson]

Thanks for your contribution!

I'd like to see another test or two, with multiple bindings to see that Injector can keep them apart. For example:

• Making sure that Annotated[int, "first"] and Annotated[int, "second"] get separate bindings/values
• Making sure that Annotated[int, "user_id"] and int get separate bindings/values

What do you think?

[ljnsn]

Thanks for the feedback! Makes sense, I will add some more tests and take a look at the failing ones.

[davidparsson]

There are a few failing tests as well, with different problems for different versions.

[ljnsn]

@davidparsson have you had time to take a look at the changes? 🙂

[ljnsn]

Sorry about that.. I've now tested with 3.7-3.11 locally and everything passes. There are now less checks to recognise Annotated in _punch_through_alias, but I think it should be fine (actually, the isinstance check is probably sufficient).

I think the mypy error is due to a newer version of mypy being used? There's no version restriction on it and I get it also on the main branch that does not have my changes. Should I ignore it?

[jstasiak]

That's an interesting feature, I've been wondering about some kind of Annotated support for a while.

I'm wondering if we're not missing some edge cases with this approach that will become obvious later, I'll have another look at this PR when I have a moment

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (44ac4e0) 95.20% compared to head (a440d70) 95.22%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   95.20%   95.22%   +0.01%     
==========================================
  Files           1        1              
  Lines         563      565       +2     
  Branches       95       96       +1     
==========================================
+ Hits          536      538       +2     
  Misses         20       20              
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ljnsn]

Have you had time to take a look yet @jstasiak ? I would like to start using this 🙂

[tobni]

This depends on this being true:

assert Annotated[int, "first"] == Annotated[int, "first"]

which I didn't know if it were or not, so I checked, and I think this holds due to _AnnotatedAlias implementing __eq__ quite intuitively (essentially does Annotated[A, B] == Annotated[C, D] if A == C and B == D).

I have checked this to be the case in python 3.10. I have also checked that the impl is order sensitive, e.g

assert Annotated[int, "first", "second"] != Annotated[int, "second", "first"]

holds.

A funny corner case due to the equality requirement is that providing Annotated[int, float("nan")] is impossible, just as Annotated[int, object()] will be.

[ljnsn]

@tobni thanks for chiming in on this.

A funny corner case due to the equality requirement is that providing Annotated[int, float("nan")] is impossible, just as Annotated[int, object()] will be.

Not sure what you mean by this, I just added a test for both of your examples and both seem to work fine.

Note that

x = float("nan")
x == x  # False

y = Annotated[int, float("nan")]
y == y  # True

[tobni]

Ah I was thinking of

Annotated[int, object()] == Annotated[int, object()] # False

Which is not an impossible (but unlikely!) scenario. In essence using bare annotations without "aliasing", and not having the metadatas __eq__ behave as intended.

But this is immaterial, the equality impl of _AnnotatedAlias is more than fine for this use case, sorry for the derail!

[ljnsn]

Ok, I see what you mean now, when passing in an annotated type with another instance in the metadata it indeed breaks down. I'd argue that's expected behaviour though, since it's clear that what you're looking up from the injector is not what you bound. It would be better to throw an error in that case though, because currently

foo = Annotated[int, object()]

def configure(binder):
    binder.bind(foo, to=123)

injector = Injector([configure])
injector.get(foo)  # returns 123
injector.get(Annotated[int, object()])  # returns 0

Not clear to my why it's returning 0 in the second case, I'll check that.

[tobni]

Probably because Annotated[int, object()]() == 0 # note the ending paren, I think that is expected behaviour? But also controversial to construct an integer with the default constructor, see #116

[jstasiak]

Very nice test coverage, I'm convinced this would be good to merge but I need to think of a thing or two first – stay tuned and thanks for your patience.

[ricardo-passthrough]

is this PR supposed to also add support for @inject? doesn't look like this case works:

Foo = Annotated[int, object()]

def configure(binder):
    binder.bind(Foo, to=123)

@inject
@dataclass
class MyClass:
    foo: Foo

injector = Injector([configure])
instance = injector.get(MyClass)
assert instance.foo == 123

It looks like @inject strips out the metadata from the type and bind to the underlying type instead

[petrkalos]

+1 to what @ricardo-passthrough mentioned, this PR doesn't work with @inject

[jstasiak]

The implementation so far has just been released in version 0.22.0.