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

Review Request #3172 - Created July 4, 2012 and submitted

Steven MacLeod
Djblets
release-0.6.x
djblets
chipx86
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.
  • 0
  • 0
  • 13
  • 1
  • 14
Description From Last Updated
Christian Hammond
  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. 
      
Steven MacLeod
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. 
      
Steven MacLeod
Mike Conley
  1. Ship It!
  2. 
      
Christian Hammond
  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. 
      
Steven MacLeod
Steven MacLeod
Christian Hammond
  1. Ship It!
  2. 
      
Steven MacLeod
Review request changed

Status: Closed (submitted)

Change Summary:

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