Skip to content
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

Comply with PEP-593 and support typing.Annotated #108

Open
roo-oliv opened this issue Aug 26, 2021 · 4 comments
Open

Comply with PEP-593 and support typing.Annotated #108

roo-oliv opened this issue Aug 26, 2021 · 4 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@roo-oliv
Copy link
Owner

roo-oliv commented Aug 26, 2021

PEP-593 defines the a standard for annotation metadata other than type hinting through the use of typing.Annotated. This framework should comply with this recommendation and give full support to typing.Annotated.

This was originally brought to attention by @Euraxluo in #107 (comment)

Initial Considerations

Currently Autowired(T) already works well with most type checkers and linters since it evaluates to T but this is not in line with the recommendation from PEP-593 to leave annotations primary to type hinting and other metadata to extras.

We should decide how best to incorporate these changes in Injectable. I intend to lay down some possible paths we can go from here in this issue thread so anyone can share their thoughts if any and then eventually I'll commit to an implementation.

While typing.Annotated was introduced in Python 3.9, this frameworks currently supports all Python versions since 3.6. So we should think in a way that will deliver the best possible experience for all possible users and use cases of Injectable.

Current State

When Python version < 3.9 then this is the only possible way to use Injectable:

@autowired
def foo(service: Autowired(Service): ...

When typing.Annotated is available (Python version >= 3.9), then this is also possible:

@autowired
def foo(service: Annotated[Autowired(Service), ...]): ...

Therefore, since Python 3.9 one can probably use other libs which rely on annotations and fully support PEP-593 without undesired interactions or further problems with the use of Injectable.

Proposals

@roo-oliv
Copy link
Owner Author

roo-oliv commented Aug 27, 2021

[REJECTED] Proposal A: Keep things the way they are

The way Autowired is implemented doesn't directly conflict with typing.Annotated and theoretically we could keep Injectable as is and everything would still work.

Use with Python versions < 3.9 remains the same:

@autowired
def foo(service: Autowired(Service)): ...

Using it with typing.Annotated would work only if the Autowired annotation is the first one:

@autowired
def foo(service: Annotated[Autowired(Service), ...]): ...

Pros

  • No need for changes
  • Less potential bugs when there are no changes

Cons

  • Not pythonic not to follow the PEP recommendation
  • It may be impeditive to work with some other libs that expect PEP-593 compliant behaviour
  • It will still be a costly and sloppy maintenance for Autowired to keep working well with linters and type checks
  • This may break expectations of new users which are accustomed to PEP-593 recommended behavior

Conclusion

I'm rejecting this approach since there seems to be no real benefits going on here and lots of problems.

@roo-oliv
Copy link
Owner Author

roo-oliv commented Aug 27, 2021

[ACCEPTED] Proposal B: Make Autowired an alias of typing.Annotated

All these uses would be correct:

@autowired
def foo(service: Autowired(Service)): ...
@autowired
def foo(service: Annotated[Autowired(Service), foo, bar]): ...
@autowired
def foo(service: Annotated[foo, Autowired(Service), bar]): ...

For backwards compatibility with Python versions prior to 3.9 we'll use the typing_extensions module which provides the Annotated generic for Python versions 3.5.3+ which is just perfect since we only support Python 3.6+.

Pros

  • We keep the Autowired use the way it already is while also being compliant to the PEP and allowing other uses
  • We need no more to care about every possible linter and type checker
  • PyCharm type inference would get along with Autowired better this way
  • We will never need to worry about interactions and conflicts with other libs which use annotations

Cons

  • None?

Conclusion

All other proposals seem inferior to this one at the moment so I'll carry with the implementation of this proposal.

@roo-oliv
Copy link
Owner Author

roo-oliv commented Aug 27, 2021

[REJECTED] Proposal C: Move Autowired from the annotations to the default values of arguments

The use of Autowired would now be like this:

@autowired
def foo(service: Service = Autowired(Service)): ...

This change can be made backwards compatible but it would cause confusion since there would be two ways to use the framework and would be painful to set and push forward a deprecation policy for the use of Autowired in annotations.

Pros

  • This simplifies some requirements of where it is allowed to declare autowired arguments (which currently is always after non-autowired args)
  • This would make the function's signature clearer and less magical (currently it is a little awkward to call foo() and see it work without passing parameters when its signature is `def foo(a, b, c):```)
  • For many cases this implementation can be more intuitive to read and use

Cons

  • Not entirely backwards compatible
  • The behavior of state can be confusing since the default value will not be actually coming from the class and be static but rather created upon object initialization
  • If one annotates a concrete type but autowire with a qualifier maybe linters will comply about Autowired's return type
  • It can be tedious to repeat the class in the type annotation and in the Autowired call; and without the repetition of the class it wouldn't work well with linters...

Conclusion

As I do not intend to change how injectable works right now and since Proposal B seems easier to implement and also to be more aligned to PEP-593 I'll reject this proposal in favor of Proposal B.

@roo-oliv
Copy link
Owner Author

roo-oliv commented Aug 27, 2021

[REJECTED] Proposal D: denote autowired arguments in the @autowired decorator, dispensing the use of Autowired

This alternative was already considered and rejected at the very beginning of this project as it has too many drawbacks but it is worth mentioning that it was given though and that this could be a way to comply with the PEP.

@autowired("service")
def foo(service: Service): ...

Pros

  • No direct interference with annotations and default values

Cons

  • Not backwards compatible
  • Passing autowiring params would be messy (@autowired({"service": {params**}})???)
  • This wouldn't be refactoring friendly if someone is changing the parameter name
  • Since the parameter name must be specified in string for this would make this process prone to typos
  • Tedious repetition of the parameters names
  • This is the uglier implementation of all, mainly when there are many args
  • Autowiring wouldn't be easily spotted at the function definition no more

Conclusion

I'm rejecting this proposal for the same reasons it was rejected in the past.

@roo-oliv roo-oliv added enhancement New feature or request help wanted Extra attention is needed labels Aug 27, 2021
@roo-oliv roo-oliv self-assigned this Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant