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

Refactor _static_request_handler #2533

Merged
merged 10 commits into from
Sep 20, 2022
Merged

Conversation

ChihweiLHBird
Copy link
Member

@ChihweiLHBird ChihweiLHBird commented Aug 20, 2022

  1. Wrap logics that processes file path to a method.
  2. Use built in update method to merge two dictionaries.
  3. Fixed an issue that when if-modified-since in request header is larger than the file modified time, 200 is still responded. 304 might be more appropriate.

@ChihweiLHBird ChihweiLHBird requested a review from ahopkins August 20, 2022 09:31
@ChihweiLHBird ChihweiLHBird requested a review from a team as a code owner August 20, 2022 09:31
@codecov
Copy link

codecov bot commented Aug 20, 2022

Codecov Report

Base: 88.425% // Head: 88.430% // Increases project coverage by +0.005% 🎉

Coverage data is based on head (81336f0) compared to base (1650331).
Patch coverage: 100.000% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##              main     #2533       +/-   ##
=============================================
+ Coverage   88.425%   88.430%   +0.005%     
=============================================
  Files           78        78               
  Lines         6471      6474        +3     
  Branches      1247      1246        -1     
=============================================
+ Hits          5722      5725        +3     
  Misses         517       517               
  Partials       232       232               
Impacted Files Coverage Δ
sanic/mixins/routes.py 93.822% <100.000%> (+0.072%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ahopkins
Copy link
Member

Not sure that this adds much.

@ChihweiLHBird
Copy link
Member Author

Not sure that this adds much.

Just added some, lol

@Tronic
Copy link
Member

Tronic commented Aug 23, 2022

Seems useful to me and it doesn't add much code, so that is also good. Header dict update is much faster than Python loop. Should keep an eye out for such loops that are better done with C builtins (perhaps there are static analyser tools that could pick up such patterns in the whole codebase?).

@ChihweiLHBird
Copy link
Member Author

Seems useful to me and it doesn't add much code, so that is also good. Header dict update is much faster than Python loop. Should keep an eye out for such loops that are better done with C builtins (perhaps there are static analyser tools that could pick up such patterns in the whole codebase?).

Thanks for the feedback. I never heard about a such tool. Let me do a search for it.

sanic/mixins/routes.py Outdated Show resolved Hide resolved
sanic/mixins/routes.py Outdated Show resolved Hide resolved
sanic/mixins/routes.py Outdated Show resolved Hide resolved
@ahopkins ahopkins merged commit 43ba381 into main Sep 20, 2022
@ahopkins ahopkins deleted the zhiwei/refactor_static_request_handler branch September 20, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants