Fix for annotated injection with attribute. Replacing with origins on… #266
Conversation
…ly injections markers but not general annotations.
|
@jstasiak pls review |
There was a problem hiding this comment.
I think this is a useful addition, but I also think that we need to consider this a breaking change. The previous implementation has its drawbacks, but if anyone relies on that, this change will break things for them. This PR will also have conflicts with #269, where the markers have been renamed to be made public.
|
@davidparsson I added a slight change to the decorators' behavior. I enabled the usage of annotated types in hints of provider and multi-provider. |
|
@sergey-zinchenko @davidparsson I have similar PR for this fix. Tests fails because github runners not executed. |
|
@sergey-zinchenko, your changes are not following the |
|
@davidparsson formatting applied |
|
@davidparsson pls re-run the pipeline. I checked on my fork, and the pipeline succeeded. |
|
@davidparsson, regarding your comment, I want to clarify that, in my opinion, this pull request is primarily a fix rather than a breaking change. The injection of annotated types partially malfunctioned, resulting in the source type being used to construct objects instead of the annotated type. It may be treated as a bug because, consequently, it was not possible to use two annotated types derived from the same source (for example, Annotated[str, "foo"] and Annotated[str, "bar"]) due to the issue that caused the last registered binding to be injected as a string rather than an annotated type. This pull request significantly enhances the usability of annotated types within the library. This fix is crucial for several of my projects, as the use of numerous annotated types is currently infeasible without it. Therefore, applying this fix is essential to ensure the proper injection of annotated types. |
|
Yes, I agree and fully understand. I don't think that there are alot of users that rely on the current/old behavor, but I can only guess, and for those users this is a breaking change, even if the old behavor was not intentional. That said, I don't see that as a reason to reject this, but rather as an input to the versioning of the next release. One could also argue that that since the previous behavior was unintentional and rather useless, it is safe to assume that nobody relies on that, and thus not consider this a breaking change. But in a strict sense, the behavior would change in a way that is not backwards-compatible. |
|
Yup. It may make sense to indicate that change may break the code based on the old behavior. |
|
@davidparsson what are the next steps to merge that PR? |
|
I would like @jstasiak's review and thoughts on how to classify this change. I think it's a reasonable change, but I don't want to make this decision myself. |
|
@davidparsson @jstasiak is there any decision on that? |
|
@jstasiak @davidparsson ping. I need that change) |
There is a problem injecting annotated type. I created a test for that to demonstrate. Shortly: we should not replace annotated type with its origin while building bindings if the annotation is not a injection marker (ie inject or noinject)