Add an Item-Content-Type header to Web API responses for list resources.

Review Request #3172 — Created July 3, 2012 and submitted

Information

Djblets
release-0.6.x

Reviewers

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 …

chipx86chipx86

Something about this sentence is making it hard for me to read. Something about the combination of the comma and …

chipx86chipx86

Should probably be styled as: allowed_mimetypes = [ {'list': mime, 'item': mime} for mime in ... ] No need for …

chipx86chipx86

Not a huge deal, but rather than loop twice, we should probably just loop once and append to each list.

chipx86chipx86

Blank line before blocks.

chipx86chipx86

Can you go into more detail as to what this code is doing? It's not immediately obvious.

chipx86chipx86

Blank line between these. Generally, I'd prefer we expand these dictionaries. It's more code, but clearer. vend_mimetype_pair = { 'list': …

chipx86chipx86

Looks like this can still be an and?

chipx86chipx86

Nit - lines 234, 236, 238, 240 and 242 are misaligned.

mike_conleymike_conley

Mis-aligned

mike_conleymike_conley

Do we really want to print the response in this test helper? Or is this leftover debugging stuff?

mike_conleymike_conley

Should have one key per line: self.allowed_mimetypes.append({ 'list': None, 'item': mimetype, })

chipx86chipx86

Same.

chipx86chipx86

This could be simplified if you set a key based on is_list. Then the supported_mimetypes assignment can be done only …

chipx86chipx86
chipx86
  1. 
      
  2. djblets/webapi/core.py (Diff revision 1)
     
     
    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.
    1. The reason I special cased this was to reduce the complexity of the change. WebAPIResource.build_response_args() (Where I generate the item_content_type) is called for every WebAPIResponse() we would possibly have item_content_type. build_response_args() is also where the mimetype for the response is generated, so it made sense to keep everything together.
      
      Moving item_content_type in to headers would complicate things quite a bit more. Would you like me to make this change, or is the special case fine?
  3. djblets/webapi/resources.py (Diff revision 1)
     
     
     
    Something about this sentence is making it hard for me to read. Something about the combination of the comma and the "respectively" maybe?
    1. How about, "Each entry in the list is a dictionary with 'list' containing a mimetype for resource lists, and 'item' containing the resource items or singletons."
  4. djblets/webapi/resources.py (Diff revision 1)
     
     
     
    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.
  5. djblets/webapi/resources.py (Diff revision 1)
     
     
     
    Not a huge deal, but rather than loop twice, we should probably just loop once and append to each list.
  6. djblets/webapi/resources.py (Diff revision 1)
     
     
    Blank line before blocks.
  7. djblets/webapi/resources.py (Diff revision 1)
     
     
    Can you go into more detail as to what this code is doing? It's not immediately obvious.
  8. djblets/webapi/resources.py (Diff revision 1)
     
     
     
    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,
    }
  9. djblets/webapi/resources.py (Diff revision 1)
     
     
     
    Looks like this can still be an and?
  10. 
      
SM
mike_conley
  1. Just a few nits that I found - nothing major. Tests pass locally too.
  2. djblets/webapi/tests.py (Diff revision 2)
     
     
    Nit - lines 234, 236, 238, 240 and 242 are misaligned.
  3. djblets/webapi/tests.py (Diff revision 2)
     
     
     
    Mis-aligned
  4. djblets/webapi/tests.py (Diff revision 2)
     
     
    Do we really want to print the response in this test helper? Or is this leftover debugging stuff?
    1. This is only printed when the test fails. Just copied from the other tests in the same file.
  5. 
      
SM
mike_conley
  1. Ship It!
  2. 
      
chipx86
  1. Couple small things remaining, and then we're done :)
    
    (Dropped the ball on this. We'll get it into the next 1.6 release.)
  2. djblets/webapi/resources.py (Diff revision 3)
     
     
    Should have one key per line:
    
    self.allowed_mimetypes.append({
        'list': None,
        'item': mimetype,
    })
  3. djblets/webapi/resources.py (Diff revision 3)
     
     
  4. djblets/webapi/resources.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
    This could be simplified if you set a key based on is_list. Then the supported_mimetypes assignment can be done only once, and use mime[key].
  5. 
      
SM
SM
chipx86
  1. Ship It!
  2. 
      
SM
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.6.x (1c76d29)
Loading...