add redirectStatus function #402
Conversation
|
Thank you @hackeryarn , much appreciate the non breaking change. Will look at this asap. |
Web/Scotty/Trans/Lazy.hs
Outdated
| -- OR | ||
| -- | ||
| -- > redirectStatus status303 "/foo/bar" | ||
| redirectStatus :: (Monad m) => Status -> T.Text -> ActionT m a |
There was a problem hiding this comment.
This is not constrained enough: one could redirect with a 200 or whatever, which would not make much sense.
There was a problem hiding this comment.
I think that running a check here and throwing the exception would create a bad user experience. It might be nicer to have a specific ‘redirectX’ functions for each status code, with the bare ‘redirect’ remaining as the 302 version. But that would require quite a few new functions.
@ocramz what are your thoughts on a path forward?
There was a problem hiding this comment.
@hackeryarn The error codes for redirect are not even that many: 300-307, 307, 308 https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#redirection_messages
I think overall providing them as individual functions would make for more solid API design (e.g redirect301, redirect302 etc.). WDYT?
There was a problem hiding this comment.
That’s the direction I was leaning to, just wanted to make sure it wouldn’t clutter the API too much.
I’ll get this updated with the new functions.
There was a problem hiding this comment.
@ocramz This should do it. I skipped codes 305 and 306 since they are both deprecated. I also moved all the redirects to their own docs section, that way it's easier for users to find everything they are looking for.
hackeryarn
force-pushed
the
redirect-status
branch
from
54b5ca7 to
40c02b6
Compare
Web/Scotty/Action.hs
Outdated
| redirect = E.throw . AERedirect | ||
| redirect = redirect302 | ||
|
|
||
| -- | Redirect to given URL with status 300. Like throwing |
There was a problem hiding this comment.
Perhaps you could add the description string to the status codes in the docstring? e.g. 300 (Multiple choices), 301 (Moved permanently) etc? For those who don't know all status codes by heart.
There was a problem hiding this comment.
Great idea @ocramz! Updated all the doc strings.
hackeryarn
force-pushed
the
redirect-status
branch
from
40c02b6 to
fa4888c
Compare
hackeryarn
force-pushed
the
redirect-status
branch
from
fa4888c to
1f98963
Compare
|
Thanks for all the reviews @ocramz. Changelog is updated and this is ready. |
This addresses issue #401.
I decided to go with a new function
redirectStatus. Doing this makes the addition a non breaking change.