Issue 17880043: Refactor post_required decorator to be more flexible. (Closed)
|
Created:
11 years, 10 months ago by iannucci Modified:
11 years, 10 months ago CC:
codereview-list_googlegroups.com Base URL:
hg::https://iannucci%40chromium.org@code.google.com/p/rietveld/@refactor_one Visibility:
Public. |
DescriptionRefactor post_required decorator to be more flexible.
Depends on https://codereview.appspot.com/17780043/
R=maruel@chromium.org
BUG=
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : upload! #MessagesTotal messages: 9
PTAL. We can start using this to lock down acceptable HTTP method(s) on any view. Live on testfoo-codereview.appspot.com (on top of previous patch) Also, git is way better than hg ^_^: https://github.com/felipec/git/blob/fc/master/git-remote-hg.py You're welcome
Sign in to reply to this message.
https://codereview.appspot.com/17880043/diff/1/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode751 codereview/views.py:751: of |methods|.""" """ on a new line https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode757 codereview/views.py:757: rsp = HttpTextResponse('This requires a specific method: %s' % allowed, use single quotes https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode764 codereview/views.py:764: You could simplify this CL a lot by adding this line: post_required = require_method('POST')
Sign in to reply to this message.
PTAL https://codereview.appspot.com/17880043/diff/1/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode751 codereview/views.py:751: of |methods|.""" On 2013/10/28 15:27:31, M-A wrote: > """ on a new line Done. https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode757 codereview/views.py:757: rsp = HttpTextResponse('This requires a specific method: %s' % allowed, On 2013/10/28 15:27:31, M-A wrote: > use single quotes Oops, done. https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode764 codereview/views.py:764: On 2013/10/28 15:27:31, M-A wrote: > You could simplify this CL a lot by adding this line: > > post_required = require_method('POST') Eh... I'd rather just have one way to do it. I don't think the indirection is helpful.
Sign in to reply to this message.
lgtm https://codereview.appspot.com/17880043/diff/1/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode764 codereview/views.py:764: On 2013/10/28 19:04:58, iannucci wrote: > On 2013/10/28 15:27:31, M-A wrote: > > You could simplify this CL a lot by adding this line: > > > > post_required = require_method('POST') > > Eh... I'd rather just have one way to do it. I don't think the indirection is > helpful. It's a trade off of: - succinctness - self-descriptiveness - ratio of common case vs uncommon case In general post_required() was the common case. Supporting multiples method is rarer, which reminds me the others should have a restriction to only GET. Overall, I don't mind much. https://codereview.appspot.com/17880043/diff/20001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/17880043/diff/20001/codereview/views.py#newcod... codereview/views.py:749: def require_method(*methods): you should name it require_methods(). Otherwise it's confusing that multiple methods are accepted.
Sign in to reply to this message.
https://codereview.appspot.com/17880043/diff/1/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/17880043/diff/1/codereview/views.py#newcode764 codereview/views.py:764: On 2013/10/28 19:47:48, M-A wrote: > On 2013/10/28 19:04:58, iannucci wrote: > > On 2013/10/28 15:27:31, M-A wrote: > > > You could simplify this CL a lot by adding this line: > > > > > > post_required = require_method('POST') > > > > Eh... I'd rather just have one way to do it. I don't think the indirection is > > helpful. > > It's a trade off of: > - succinctness > - self-descriptiveness > - ratio of common case vs uncommon case > > In general post_required() was the common case. Supporting multiples method is > rarer, which reminds me the others should have a restriction to only GET. > Overall, I don't mind much. Yeah I was assuming that we would have a later patch which locks down everything to the correct supported methods, but I didn't want to do it in the same CL. https://codereview.appspot.com/17880043/diff/20001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/17880043/diff/20001/codereview/views.py#newcod... codereview/views.py:749: def require_method(*methods): On 2013/10/28 19:47:48, M-A wrote: > you should name it require_methods(). Otherwise it's confusing that multiple > methods are accepted. Done.
Sign in to reply to this message.
On 2013/10/28 23:36:48, iannucci wrote: > Done. you forgot to upload.
Sign in to reply to this message.
On 2013/10/29 00:21:57, M-A wrote: > On 2013/10/28 23:36:48, iannucci wrote: > > Done. > > you forgot to upload. uploaded (includes a rebase)
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
Message was sent while issue was closed.
Committed as https://code.google.com/p/rietveld/source/detail?r=229b7b214391221cf18e1ab5b3... (default) Merged as https://code.google.com/p/rietveld/source/detail?r=3447b76bf89ab2ad8325e94ce1... (chromium)
Sign in to reply to this message.
|
