-
-
I think, given that WebAPIResponse doesn't necessarily mean you're using resources, that we shouldn't special-case this. Instead, we should just stick it in the headers passed to this.
-
Something about this sentence is making it hard for me to read. Something about the combination of the comma and the "respectively" maybe?
-
Should probably be styled as: allowed_mimetypes = [ {'list': mime, 'item': mime} for mime in ... ] No need for the list(), and generall the value and the for in a generator are aligned.
-
Not a huge deal, but rather than loop twice, we should probably just loop once and append to each list.
-
-
-
Blank line between these. Generally, I'd prefer we expand these dictionaries. It's more code, but clearer. vend_mimetype_pair = { 'list': None, 'item': None, }
-
Add an Item-Content-Type header to Web API responses for list resources.
Review Request #3172 — Created July 3, 2012 and submitted
Added an Item-Content-Type header to Web API responses for list resources. This header provides the mime-type for the item resource of the list. Introduces a new 'allowed_mimetypes' attribute to WebAPIResources, and starts the process of deprecating 'allowed_list_mimetypes', and 'allowed_item_mimetypes'. Any items missing from 'allowed_mimetypes' found in 'allowed_list_mimetypes', or 'allowed_item_mimetypes' will be appended during initialization to be backwards compatible.
Wrote new test cases to check the generated Item-Content-Type header, ran all Djblets tests, and manually tested in a web browser.
Description | From | Last Updated |
---|---|---|
I think, given that WebAPIResponse doesn't necessarily mean you're using resources, that we shouldn't special-case this. Instead, we should just … |
chipx86 | |
Something about this sentence is making it hard for me to read. Something about the combination of the comma and … |
chipx86 | |
Should probably be styled as: allowed_mimetypes = [ {'list': mime, 'item': mime} for mime in ... ] No need for … |
chipx86 | |
Not a huge deal, but rather than loop twice, we should probably just loop once and append to each list. |
chipx86 | |
Blank line before blocks. |
chipx86 | |
Can you go into more detail as to what this code is doing? It's not immediately obvious. |
chipx86 | |
Blank line between these. Generally, I'd prefer we expand these dictionaries. It's more code, but clearer. vend_mimetype_pair = { 'list': … |
chipx86 | |
Looks like this can still be an and? |
chipx86 | |
Nit - lines 234, 236, 238, 240 and 242 are misaligned. |
mike_conley | |
Mis-aligned |
mike_conley | |
Do we really want to print the response in this test helper? Or is this leftover debugging stuff? |
mike_conley | |
Should have one key per line: self.allowed_mimetypes.append({ 'list': None, 'item': mimetype, }) |
chipx86 | |
Same. |
chipx86 | |
This could be simplified if you set a key based on is_list. Then the supported_mimetypes assignment can be done only … |
chipx86 |
- Change Summary:
-
Updated based on Christian's review. Also fixed a small typo in the new tests.
- Diff:
-
Revision 2 (+178 -16)
- Change Summary:
-
Changed description to remove note about patch breaking review board. The required patch to the FileDiffResource is now in the repo.
- Description:
-
Added an Item-Content-Type header to Web API responses for list resources. This header provides the mime-type for the item resource of the list.
Introduces a new 'allowed_mimetypes' attribute to WebAPIResources, and starts the process of deprecating 'allowed_list_mimetypes', and 'allowed_item_mimetypes'. Any items missing from 'allowed_mimetypes' found in 'allowed_list_mimetypes', or 'allowed_item_mimetypes' will be appended during initialization to be backwards compatible.
- - This change causes Review Board to fail, due to a naming conflict with allowed_mimetypes in the DiffResource. A separate patch must be applied to Review Board to rename DiffResource.allowed_mimetypes, and prevent collision.