Skip to content

Comments

HTTP cache methods#129

Open
JindrichPilar wants to merge 6 commits intoklein:masterfrom
JindrichPilar:master
Open

HTTP cache methods#129
JindrichPilar wants to merge 6 commits intoklein:masterfrom
JindrichPilar:master

Conversation

@JindrichPilar
Copy link

HTTP cache API added publicCache(), privateCache(), noCache()

publicCache(), privateCache(), sendCacheHeaders() added
noCache() refactored
tests added
@Rican7
Copy link
Member

Rican7 commented Aug 6, 2013

@jinora this is a great pull request! I appreciate you including tests also.

A few things though. I'll comment on the code lines where appropriate. Otherwise, HTTP caching requires many different pieces and directives, as @chriso points out here.

Copy link
Member

Choose a reason for hiding this comment

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

The new constants should probably just be integers. They're values don't really have much significance, since they're meant to be compared as constants, so following the PHP (Java inspired) convention of simply assigning an integer value would be better suited here.

Copy link
Author

Choose a reason for hiding this comment

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

String constant significance lies in debugging. Where string can suggest flags meaning without having to look at constant list. My mistake, I overlooked that DISPATCH constants use integers. Will cahnge to integers.

@chriso
Copy link
Contributor

chriso commented Aug 7, 2013

It looks fine to merge, but whether or not this belongs in this library (a microframework) is debatable.

Up to you @Rican7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants