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

Fixing typing for ListenerMixin.listener #2376

Merged
merged 5 commits into from
Mar 23, 2022

Conversation

aericson
Copy link
Contributor

Description

This PR addresses #2375. The typing of that method is a bit complex because it has two different code paths which result in different typing of parameters and return.

I've added two overloads so that we get better type checking.

Alternative solutions

I've also considered another way to go around this by separating listener into two methods which should reduce complexity but in this PR I only addressed the typing.

Here is how the alternative solution could look like:
https://gist.github.com/aericson/d36514eb45b9ec1f65b8ee51fdf5c08f

@aericson aericson marked this pull request as ready for review January 14, 2022 21:44
@aericson aericson requested a review from a team as a code owner January 14, 2022 21:44
@ahopkins
Copy link
Member

I've also considered another way to go around this by separating listener into two methods which should reduce complexity but in this PR I only addressed the typing.

Let's consider that in a separate PR.

sanic/mixins/listeners.py Outdated Show resolved Hide resolved
sanic/mixins/listeners.py Outdated Show resolved Hide resolved
sanic/mixins/listeners.py Outdated Show resolved Hide resolved
@aericson aericson requested a review from ahopkins January 16, 2022 11:40
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #2376 (0ae661a) into main (6e0a687) will decrease coverage by 0.073%.
The diff coverage is 84.615%.

@@              Coverage Diff              @@
##              main     #2376       +/-   ##
=============================================
- Coverage   87.183%   87.109%   -0.074%     
=============================================
  Files           60        60               
  Lines         5017      5027       +10     
  Branches       904       905        +1     
=============================================
+ Hits          4374      4379        +5     
- Misses         470       475        +5     
  Partials       173       173               
Impacted Files Coverage Δ
sanic/mixins/listeners.py 96.428% <81.818%> (-3.572%) ⬇️
sanic/models/handler_types.py 100.000% <100.000%> (ø)
sanic/app.py 89.147% <0.000%> (-0.582%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e0a687...0ae661a. Read the comment docs.

@ahopkins
Copy link
Member

Alternative solutions

I've also considered another way to go around this by separating listener into two methods which should reduce complexity but in this PR I only addressed the typing.

No rush on this, but do you think you can put this alternative version together as a PR for the June release?

@aericson
Copy link
Contributor Author

Alternative solutions

I've also considered another way to go around this by separating listener into two methods which should reduce complexity but in this PR I only addressed the typing.

No rush on this, but do you think you can put this alternative version together as a PR for the June release?

Yep, I can put some time into that. Are you thinking on us closing this PR and having a new one with that? Just wondering if I should do on top of this changes or from a clean branch.

@ahopkins
Copy link
Member

ahopkins commented Mar 23, 2022

Yep, I can put some time into that. Are you thinking on us closing this PR and having a new one with that? Just wondering if I should do on top of this changes or from a clean branch.

As a new PR separate from this. I am getting the tests to pass and will merge this one in today for the release next week. The other change I think is good, but let's hold off until next cycle (unless you get it done in the next day or so, then we can include it also).

@ahopkins ahopkins merged commit 32962d1 into sanic-org:main Mar 23, 2022
@aericson aericson deleted the fix-listener-typing branch March 23, 2022 14:38
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
Co-authored-by: Adam Hopkins <adam@amhopkins.com>
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