Conversation
yoshuawuyts
force-pushed
the
cache-control
branch
from
909cbf0 to
8e23418
Compare
yoshuawuyts
force-pushed
the
cache-control
branch
from
42cdb0c to
60a5cd9
Compare
| } | ||
|
|
||
| /// Create a new instance from headers. | ||
| pub fn from_headers(headers: impl AsRef<Headers>) -> crate::Result<Option<Self>> { |
There was a problem hiding this comment.
I thought from_headers() was supposed to just be Option<Self>?
And this doesn't actually seem to make use of Result?
(Although I think that in many cases Result<Option<Header>> will probably make sense but is so far inconsistent...)
There was a problem hiding this comment.
from_headers needs to be able to both detect whether a header was found, and whether parsing the header was successful. These are semantically different, and should be handled separately.
However in #99 an argument is made to use Option<Result<Self>> instead, which may actually be better. We should probably adjust all existing from_header methods to this shape, and update this one too (the only existing ones are currently behind an unstable flag, so we're okay).
There was a problem hiding this comment.
On further thought I think that Result<Option<T>> should probably always be preferred, because it allows a seamless transition to e.g. ~ -> Option<T> throws Result if that ever becomes a thing.
There was a problem hiding this comment.
The right signature would be -> Option<T> throws Error, but even then it'd be possible to write -> crate::Result<T> throws NoneError. I don't think the possibility of throws is something we should consider as a constraint for any design now. It's unclear if we'll ever have it, and even then the semantics are still undefined.
Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>
| } | ||
|
|
||
| /// Create a new instance from headers. | ||
| pub fn from_headers(headers: impl AsRef<Headers>) -> crate::Result<Option<Self>> { |
There was a problem hiding this comment.
from_headers needs to be able to both detect whether a header was found, and whether parsing the header was successful. These are semantically different, and should be handled separately.
However in #99 an argument is made to use Option<Result<Self>> instead, which may actually be better. We should probably adjust all existing from_header methods to this shape, and update this one too (the only existing ones are currently behind an unstable flag, so we're okay).
|
Going to merge this + the other PRs, and then do a pass in a separate PR to convert edit: coming around on that idea; implemented this for several instances of |
2 participants
Started work on Cache-Control headers, as per https://developer.mozilla.org/en-US/docs/Web/HTTP/Caching. I feel like caching is something that could be really interesting to get right for Tide, and probably getting the typed headers in place is the first step to experiment with building more interesting abstractions.