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

typing: add middleware protocols #2390

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jkmnt
Copy link

@jkmnt jkmnt commented Oct 26, 2024

Summary of Changes

Typed the middleware classes with union of protocols.
Middleware class may conform to any protocol of the union to pass the type check

It's not perfect due the nature of python's static duck typing, but better than the plain object ) Class may have correct signature for process_request(), but wrong signature for process_response(). It would be accepted since MiddlewareWithProcessRequest says nothing about the process_response.

But if user explicitly states implemented protocols, then the typecheker will ensure strict conformance.

class MyMiddleware(MiddlewareWithProcessRequest, MiddlewareWithProcessResponse):
    def process_request(...): ... # ok, pass
    def process_responce(...):  # won't pass (typo). process_response is required 

The protocols definitions are in the private falcon._typing module now. Should they be moved to the public falcon.typing to support this usecase ?

The protocols names are long and noisy. Use some abbreviations ?

Related Issues

#2372

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (b541976) to head (4e8eea2).

Additional details and impacted files
@@             Coverage Diff             @@
##            master    #2390      +/-   ##
===========================================
- Coverage   100.00%   99.92%   -0.08%     
===========================================
  Files           64       64              
  Lines         7726     7730       +4     
  Branches      1071     1071              
===========================================
- Hits          7726     7724       -2     
- Misses           0        5       +5     
- Partials         0        1       +1     

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


🚨 Try these New Features:

@jkmnt jkmnt marked this pull request as ready for review October 29, 2024 11:11
@vytas7
Copy link
Member

vytas7 commented Nov 22, 2024

One thing that we need to investigate (not necessarily related to this change) is how to control annotations in Sphinx. Because if this new union of protocols gets evaluated/expanded, it really clutters the docs... 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants