[checklist] Refactoring of checklist

Review Request #7176 — Created April 7, 2015 and submitted

Information

rb-extension-pack
2124492...

Reviewers

Refactoring includes

  • Use of request.user instead of passing user_id in every HTTP request
  • Updating of the view only after successful API response
  • Renamed status of each item from finished to checked
  • Use of ChecklistItem non-database resource API instead of using HTTP request parameters
  • Moving of front-end API logic from view to model
  • CSS hyphens everywhere instead of underscores
  • Improving Python code (renaming variables, removing redundant code)
  • Start checklist item IDs from 0 instead of 1.

Basic CRUD functionality remains. Ensured that the views will not update unless there is a successful response from the server.

Description From Last Updated

'resources' imported but unused

reviewbotreviewbot

Col: 25 E261 at least two spaces before inline comment

reviewbotreviewbot

local variable 'item' is assigned to but never used

reviewbotreviewbot

'webapi_response_errors' imported but unused

reviewbotreviewbot

'ChecklistItem' imported but unused

reviewbotreviewbot

Col: 53 E261 at least two spaces before inline comment

reviewbotreviewbot

The first line of this file should be: from __future__ import unicode_literals

brenniebrennie

Can you put these into alphabetical order?

brenniebrennie

Undo this change.

brenniebrennie

The first line of this file should be: from __future__ import unicode_literals

brenniebrennie

Undo this change. django, djblets, and reviewboard are all third party imports with repsect to rb-extension-pack.

brenniebrennie

Docstrings should be of the following form: """Single line summary Multi-line description. """

brenniebrennie

If we're going to put a code example in the docstring, we should be using a restructured text code block.

brenniebrennie

"A JSON blob"

brenniebrennie

This should probably be six.text_type (from django.utils.six), not str. If this should be a byte string type, use six.binary_type instead.

brenniebrennie

Same comment about six.text_type vs str applies here.

brenniebrennie

You don't need pass if the class has a docstring.

brenniebrennie

Same comment here about unicode_literals.

brenniebrennie

Same comment about unicode_literals as above.

brenniebrennie

Blank line between these.

brenniebrennie

Shouldn't this be int ? The model indicates that ids are integers.

brenniebrennie

Missing trailing comma.

brenniebrennie

This should be six.text_type.

brenniebrennie

Missing trailing comma.

brenniebrennie

Missing trailing comma.

brenniebrennie

Missing trailing comma.

brenniebrennie

Can you catch CheckListItem.DoesNotExist here instead?

brenniebrennie

six.text_type, not str

brenniebrennie

six.text_type

brenniebrennie

Can you catch CheckListItem.DoesNotExist here instead?

brenniebrennie

six.text_type

brenniebrennie

Should have a trailing comma.

brenniebrennie

Trailing comma

brenniebrennie

Trailing comma.

brenniebrennie

Can you catch CheckListItem.DoesNotExist here instead?

brenniebrennie

You can add checklist_item_id as a paramter to the function and just refer to it that way. Its cleaner than …

brenniebrennie

This shoudl have from __future__ import unicode_literals

brenniebrennie

"ID"

brenniebrennie

Is this left over from debugging?

brenniebrennie

This should be formatted the same as a Python docstring. This applies to all methods.

brenniebrennie

Can we just use response.checklist_item.keys() here?

brenniebrennie

I feel like since we're using this all over the file, and we always have access to the Checklist object, …

brenniebrennie

"The"

brenniebrennie

review_request_id shouldn't be quoted.

brenniebrennie

How about _addItemToView ?

brenniebrennie

I think it would be ok to include the keys we expect in there (id, checked, description ?).

brenniebrennie

Won't this start indexing from 1? Shouldn't we index from 0?

brenniebrennie

I don't believe you should be using a dummy model for the web API. Models are not required.

brenniebrennie

This will end up in public-facing documentation.

brenniebrennie

We don't really need this.

brenniebrennie

Can we put a doc comment on this?

brenniebrennie

Doc comments should be of the form /* * Single line summary. * * Multi-line description. */ Same elsewhere.

brenniebrennie

Needs a doc comment.

brenniebrennie

Needs long-form doc comment.

brenniebrennie

It technically isn't a dictionary -- its a JS object. You could call it a mapping, instead.

brenniebrennie

Needs doc comment.

brenniebrennie

Needs doc comment.

brenniebrennie

Should use long form doc comment. Same elsewhere.

brenniebrennie

If you are referring to Python data types, this is actually a six.text_type.

brenniebrennie

Same here.

brenniebrennie

Can you fix up this docstring?

brenniebrennie
SU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
  2.  'resources' imported but unused
    
  3. Col: 25
     E261 at least two spaces before inline comment
    
  4.  local variable 'item' is assigned to but never used
    
  5.  'webapi_response_errors' imported but unused
    
  6.  'ChecklistItem' imported but unused
    
  7. Col: 53
     E261 at least two spaces before inline comment
    
  8. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
  2. 
      
brennie
  1. <p>I'm confused. The model has the id fields for the checklist items as integers, but you convert them to strings all over the place. Why aren't we just using integers?</p>

    1. Sorry, the comment in the model is incorrect, checklist item IDs should be strings at the moment. It was already a str when I took over this project. I plan on keeping the string type for IDs especially for nested items. For example, id: '2_1' will mean that it's an item 1 nested under an item 2. Alternatively, IDs as integers work too, but I will not rule out the possibility of using strings yet.

    2. Ah! This makes sense. Strings will work fine in this case.

  2. checklist/checklist/extension.py (Diff revision 3)
     
     

    The first line of this file should be:

    from __future__ import unicode_literals

  3. checklist/checklist/extension.py (Diff revision 3)
     
     

    Can you put these into alphabetical order?

  4. checklist/checklist/extension.py (Diff revision 3)
     
     

    Undo this change.

  5. checklist/checklist/models.py (Diff revision 3)
     
     

    The first line of this file should be:

    from __future__ import unicode_literals

  6. checklist/checklist/models.py (Diff revision 3)
     
     

    Undo this change. django, djblets, and reviewboard are all third party imports with repsect to rb-extension-pack.

  7. checklist/checklist/models.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Docstrings should be of the following form:

    """Single line summary

    Multi-line description.
    """

  8. checklist/checklist/models.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    If we're going to put a code example in the docstring, we should be using a restructured text code block.

  9. checklist/checklist/models.py (Diff revision 3)
     
     

    "A JSON blob"

  10. checklist/checklist/models.py (Diff revision 3)
     
     
     

    This should probably be six.text_type (from django.utils.six), not str. If this should be a byte string type, use six.binary_type instead.

  11. checklist/checklist/models.py (Diff revision 3)
     
     
     

    Same comment about six.text_type vs str applies here.

  12. checklist/checklist/models.py (Diff revision 3)
     
     

    You don't need pass if the class has a docstring.

  13. Same comment here about unicode_literals.

  14. Same comment about unicode_literals as above.

  15. Blank line between these.

  16. Shouldn't this be int ?

    The model indicates that ids are integers.

  17. Missing trailing comma.

  18. This should be six.text_type.

  19. Missing trailing comma.

  20. Missing trailing comma.

  21. Missing trailing comma.

  22. Can you catch CheckListItem.DoesNotExist here instead?

    1. Just using this comment as a reminder (checklist item? hah) that this should probably be Checklist.DoesNotExist.

  23. six.text_type, not str

  24. Can you catch CheckListItem.DoesNotExist here instead?

  25. Should have a trailing comma.

  26. Trailing comma

  27. Trailing comma.

  28. Can you catch CheckListItem.DoesNotExist here instead?

  29. You can add checklist_item_id as a paramter to the function and just refer to it that way. Its cleaner than indexing into kwargs. Same elsewhere.

  30. This shoudl have from __future__ import unicode_literals

  31. Is this left over from debugging?

    1. I meant for this to be an assertion, although it does not check if this.collection is null, which sometimes causes the statement to error instead of being asserted. Apart from that, is the assertion being used correctly in this case?

    2. If its sometimes causing with an error with this.collection being null, that should probably be checked, too.

      I'm not sure if we leave asserts in our client JS code; you should probably ask David or Christian about this.

      If this does get left in, make sure to move the var statement above it; the actual assignment can be done below it.

    3. I will assume that asserts are allowed in client JS code for now.

  32. This should be formatted the same as a Python docstring. This applies to all methods.

  33. Can we just use response.checklist_item.keys() here?

    1. The map function returns the values instead of keys. Which also means I can use _.values(response.checklist_item) from Underscore.js. /reminder

  34. I feel like since we're using this all over the file, and we always have access to the Checklist object, that this should be a constant on the parent Checklist object.

  35. review_request_id shouldn't be quoted.

  36. How about _addItemToView ?

  37. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
  2. 
      
SU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
  2. 
      
brennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 5)
     
     

    I think it would be ok to include the keys we expect in there (id, checked, description ?).

  3. checklist/checklist/models.py (Diff revision 5)
     
     
     
     
     
     
     
     

    Won't this start indexing from 1? Shouldn't we index from 0?

  4. checklist/checklist/models.py (Diff revision 5)
     
     
     

    I don't believe you should be using a dummy model for the web API. Models are not required.

  5. This will end up in public-facing documentation.

  6. We don't really need this.

  7. Can we put a doc comment on this?

  8. Doc comments should be of the form

    /*
     * Single line summary.
     * 
     * Multi-line description.
     */
    

    Same elsewhere.

  9. Needs a doc comment.

  10. Needs long-form doc comment.

  11. It technically isn't a dictionary -- its a JS object. You could call it a mapping, instead.

  12. Needs doc comment.

  13. Needs doc comment.

  14. Should use long form doc comment. Same elsewhere.

  15. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
  2. 
      
brennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 6)
     
     

    If you are referring to Python data types, this is actually a six.text_type.

  3. checklist/checklist/models.py (Diff revision 6)
     
     

    Same here.

  4. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
  2. 
      
brennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 7)
     
     
     
     

    Can you fix up this docstring?

  3. 
      
SU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/models.py
        checklist/checklist/resources/checklist_item.py
        checklist/checklist/extension.py
        checklist/checklist/resources/__init__.py
        checklist/checklist/resources/checklist_resource.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
        checklist/checklist/static/js/models/checklistAPI.js
        checklist/checklist/static/css/style.less
        checklist/checklist/static/js/models/checklist.js
        checklist/checklist/templates/checklist/template.html
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
SU
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (27afa78)
Loading...