Change Summary:
Add comments and documentation. And yay to fixing of the parent-diff bug!
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+417 -337) |
Review Request #7176 — Created April 7, 2015 and submitted
Refactoring includes
request.user
instead of passing user_id
in every HTTP requestfinished
to checked
ChecklistItem
non-database resource API instead of using HTTP request parametersBasic 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 |
reviewbot | |
Col: 25 E261 at least two spaces before inline comment |
reviewbot | |
local variable 'item' is assigned to but never used |
reviewbot | |
'webapi_response_errors' imported but unused |
reviewbot | |
'ChecklistItem' imported but unused |
reviewbot | |
Col: 53 E261 at least two spaces before inline comment |
reviewbot | |
The first line of this file should be: from __future__ import unicode_literals |
brennie | |
Can you put these into alphabetical order? |
brennie | |
Undo this change. |
brennie | |
The first line of this file should be: from __future__ import unicode_literals |
brennie | |
Undo this change. django, djblets, and reviewboard are all third party imports with repsect to rb-extension-pack. |
brennie | |
Docstrings should be of the following form: """Single line summary Multi-line description. """ |
brennie | |
If we're going to put a code example in the docstring, we should be using a restructured text code block. |
brennie | |
"A JSON blob" |
brennie | |
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. |
brennie | |
Same comment about six.text_type vs str applies here. |
brennie | |
You don't need pass if the class has a docstring. |
brennie | |
Same comment here about unicode_literals. |
brennie | |
Same comment about unicode_literals as above. |
brennie | |
Blank line between these. |
brennie | |
Shouldn't this be int ? The model indicates that ids are integers. |
brennie | |
Missing trailing comma. |
brennie | |
This should be six.text_type. |
brennie | |
Missing trailing comma. |
brennie | |
Missing trailing comma. |
brennie | |
Missing trailing comma. |
brennie | |
Can you catch CheckListItem.DoesNotExist here instead? |
brennie | |
six.text_type, not str |
brennie | |
six.text_type |
brennie | |
Can you catch CheckListItem.DoesNotExist here instead? |
brennie | |
six.text_type |
brennie | |
Should have a trailing comma. |
brennie | |
Trailing comma |
brennie | |
Trailing comma. |
brennie | |
Can you catch CheckListItem.DoesNotExist here instead? |
brennie | |
You can add checklist_item_id as a paramter to the function and just refer to it that way. Its cleaner than … |
brennie | |
This shoudl have from __future__ import unicode_literals |
brennie | |
"ID" |
brennie | |
Is this left over from debugging? |
brennie | |
This should be formatted the same as a Python docstring. This applies to all methods. |
brennie | |
Can we just use response.checklist_item.keys() here? |
brennie | |
I feel like since we're using this all over the file, and we always have access to the Checklist object, … |
brennie | |
"The" |
brennie | |
review_request_id shouldn't be quoted. |
brennie | |
How about _addItemToView ? |
brennie | |
I think it would be ok to include the keys we expect in there (id, checked, description ?). |
brennie | |
Won't this start indexing from 1? Shouldn't we index from 0? |
brennie | |
I don't believe you should be using a dummy model for the web API. Models are not required. |
brennie | |
This will end up in public-facing documentation. |
brennie | |
We don't really need this. |
brennie | |
Can we put a doc comment on this? |
brennie | |
Doc comments should be of the form /* * Single line summary. * * Multi-line description. */ Same elsewhere. |
brennie | |
Needs a doc comment. |
brennie | |
Needs long-form doc comment. |
brennie | |
It technically isn't a dictionary -- its a JS object. You could call it a mapping, instead. |
brennie | |
Needs doc comment. |
brennie | |
Needs doc comment. |
brennie | |
Should use long form doc comment. Same elsewhere. |
brennie | |
If you are referring to Python data types, this is actually a six.text_type. |
brennie | |
Same here. |
brennie | |
Can you fix up this docstring? |
brennie |
Add comments and documentation. And yay to fixing of the parent-diff bug!
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+417 -337) |
checklist/checklist/resources/checklist_item.py (Diff revision 2) |
---|
Col: 25 E261 at least two spaces before inline comment
checklist/checklist/resources/checklist_item.py (Diff revision 2) |
---|
local variable 'item' is assigned to but never used
checklist/checklist/resources/checklist_resource.py (Diff revision 2) |
---|
'webapi_response_errors' imported but unused
checklist/checklist/resources/checklist_resource.py (Diff revision 2) |
---|
'ChecklistItem' imported but unused
checklist/checklist/resources/checklist_resource.py (Diff revision 2) |
---|
Col: 53 E261 at least two spaces before inline comment
Fix ReviewBot issues.
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
<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>
checklist/checklist/extension.py (Diff revision 3) |
---|
The first line of this file should be:
from __future__ import unicode_literals
checklist/checklist/models.py (Diff revision 3) |
---|
The first line of this file should be:
from __future__ import unicode_literals
checklist/checklist/models.py (Diff revision 3) |
---|
Undo this change.
django
,djblets
, andreviewboard
are all third party imports with repsect torb-extension-pack
.
checklist/checklist/models.py (Diff revision 3) |
---|
Docstrings should be of the following form:
"""Single line summary
Multi-line description.
"""
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.
checklist/checklist/models.py (Diff revision 3) |
---|
This should probably be
six.text_type
(from django.utils.six
), notstr
. If this should be a byte string type, usesix.binary_type
instead.
checklist/checklist/models.py (Diff revision 3) |
---|
Same comment about
six.text_type
vsstr
applies here.
checklist/checklist/resources/__init__.py (Diff revision 3) |
---|
Same comment here about
unicode_literals
.
checklist/checklist/resources/checklist_item.py (Diff revision 3) |
---|
Same comment about
unicode_literals
as above.
checklist/checklist/resources/checklist_item.py (Diff revision 3) |
---|
Shouldn't this be
int
?The model indicates that ids are integers.
checklist/checklist/resources/checklist_item.py (Diff revision 3) |
---|
Can you catch
CheckListItem.DoesNotExist
here instead?
checklist/checklist/resources/checklist_item.py (Diff revision 3) |
---|
Can you catch
CheckListItem.DoesNotExist
here instead?
checklist/checklist/resources/checklist_item.py (Diff revision 3) |
---|
Can you catch
CheckListItem.DoesNotExist
here instead?
checklist/checklist/resources/checklist_item.py (Diff revision 3) |
---|
You can add
checklist_item_id
as a paramter to the function and just refer to it that way. Its cleaner than indexing intokwargs
. Same elsewhere.
checklist/checklist/resources/checklist_resource.py (Diff revision 3) |
---|
This shoudl have
from __future__ import unicode_literals
checklist/checklist/static/js/models/checklist.js (Diff revision 3) |
---|
Is this left over from debugging?
checklist/checklist/static/js/models/checklist.js (Diff revision 3) |
---|
This should be formatted the same as a Python docstring. This applies to all methods.
checklist/checklist/static/js/models/checklist.js (Diff revision 3) |
---|
Can we just use
response.checklist_item.keys()
here?
checklist/checklist/static/js/models/checklist.js (Diff revision 3) |
---|
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 parentChecklist
object.
checklist/checklist/static/js/views/checklistView.js (Diff revision 3) |
---|
review_request_id
shouldn't be quoted.
Fix trivial issues.
Diff: |
Revision 4 (+427 -337)
|
---|
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
Use
six.text_type
instead ofstr
.
Use reStructuredText in comment block.
Improvements for comments, variables and Python method arguments.
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+438 -339)
|
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
checklist/checklist/models.py (Diff revision 5) |
---|
I think it would be ok to include the keys we expect in there (
id
,checked
,description
?).
checklist/checklist/models.py (Diff revision 5) |
---|
Won't this start indexing from 1? Shouldn't we index from 0?
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.
checklist/checklist/resources/checklist_item.py (Diff revision 5) |
---|
This will end up in public-facing documentation.
checklist/checklist/static/js/models/checklist.js (Diff revision 5) |
---|
Can we put a doc comment on this?
checklist/checklist/static/js/models/checklist.js (Diff revision 5) |
---|
Doc comments should be of the form
/* * Single line summary. * * Multi-line description. */
Same elsewhere.
checklist/checklist/static/js/models/checklist.js (Diff revision 5) |
---|
It technically isn't a dictionary -- its a JS object. You could call it a mapping, instead.
checklist/checklist/static/js/views/checklistView.js (Diff revision 5) |
---|
Should use long form doc comment. Same elsewhere.
Update comments and docs.
Checklist item indices now starts from 0 instead of 1.
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+452 -339)
|
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
checklist/checklist/models.py (Diff revision 6) |
---|
If you are referring to Python data types, this is actually a
six.text_type
.
Six is lucky seven ate nine.
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+452 -339)
|
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
Fix docstring.
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+457 -341)
|
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