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

File Cache Control Headers Support #2447

Merged
merged 22 commits into from Jun 16, 2022

Conversation

ChihweiLHBird
Copy link
Member

Related issue: #2441

Would like to get some feedback about the implementation.

The behavior was made to be similar to Werkzeug. But I think we can only keep the auto headers option (and remove last_modified) since it is a fact from the file stat and user can still override the it with a headers dictionary passed in.

@ChihweiLHBird ChihweiLHBird changed the title cache headers support File Cache Headers Support May 3, 2022
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #2447 (035bbd2) into main (2f90a85) will increase coverage by 0.000%.
The diff coverage is 88.888%.

@@            Coverage Diff            @@
##              main     #2447   +/-   ##
=========================================
  Coverage   87.246%   87.247%           
=========================================
  Files           60        60           
  Lines         5073      5089   +16     
  Branches       907       912    +5     
=========================================
+ Hits          4426      4440   +14     
  Misses         476       476           
- Partials       171       173    +2     
Impacted Files Coverage Δ
sanic/response.py 89.230% <88.888%> (-0.155%) ⬇️

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 2f90a85...035bbd2. Read the comment docs.

@ChihweiLHBird ChihweiLHBird force-pushed the zhiwei/cache-headers branch 2 times, most recently from 3f8cfb4 to d18b286 Compare May 16, 2022 08:50
@ChihweiLHBird
Copy link
Member Author

ChihweiLHBird commented May 16, 2022

Currently, the headers look like:
image

@ChihweiLHBird
Copy link
Member Author

@sanic-org/framework @ahopkins May I have some feedbacks about the draft? Thanks!

@Tronic
Copy link
Member

Tronic commented May 17, 2022

@ChihweiLHBird Please avoid force pushing on PRs. Use normal push so that the notification don't give an error about commits not being found.

@ChihweiLHBird
Copy link
Member Author

@ChihweiLHBird Please avoid force pushing on PRs. Use normal push so that the notification don't give an error about commits not being found.

Thanks. Sorry about that, and I didn't know that before. I wanted to undo the commit because the change was messed up. Won't do it in the future PRs.

@ChihweiLHBird ChihweiLHBird marked this pull request as ready for review May 18, 2022 21:34
@ChihweiLHBird ChihweiLHBird requested a review from a team as a code owner May 18, 2022 21:34
@ChihweiLHBird ChihweiLHBird marked this pull request as draft May 18, 2022 22:26
@ChihweiLHBird ChihweiLHBird marked this pull request as ready for review May 19, 2022 08:41
sanic/response.py Outdated Show resolved Hide resolved
sanic/response.py Outdated Show resolved Hide resolved
sanic/response.py Outdated Show resolved Hide resolved
sanic/response.py Outdated Show resolved Hide resolved
sanic/response.py Outdated Show resolved Hide resolved
@ahopkins ahopkins marked this pull request as draft May 24, 2022 06:03
@ahopkins
Copy link
Member

Marking it as a draft. Change it back when it is ready for final review 😎

@ChihweiLHBird ChihweiLHBird marked this pull request as ready for review May 26, 2022 06:51
@ChihweiLHBird ChihweiLHBird changed the title File Cache Headers Support File Cache Control Headers Support May 26, 2022
Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good first step, but I wonder if we should raise a warning or something if cache-control was set in the headers? Will merge as is, but we can think about this for the next round.

@ahopkins ahopkins merged commit a744041 into sanic-org:main Jun 16, 2022
@ChihweiLHBird ChihweiLHBird deleted the zhiwei/cache-headers branch June 16, 2022 18:41
@ChihweiLHBird
Copy link
Member Author

This is a good first step, but I wonder if we should raise a warning or something if cache-control was set in the headers? Will merge as is, but we can think about this for the next round.

How to? Maybe mention it in the docs? Or have a parameter called something like enable_auto_cache_control?

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.

None yet

3 participants