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
Information | |
---|---|
Sunxperous | |
rb-extension-pack | |
|
|
7248 | |
2124492... | |
Reviewers | |
rb-extension-pack, students | |
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 |
![]() |
|
Col: 25 E261 at least two spaces before inline comment |
![]() |
|
local variable 'item' is assigned to but never used |
![]() |
|
'webapi_response_errors' imported but unused |
![]() |
|
'ChecklistItem' imported but unused |
![]() |
|
Col: 53 E261 at least two spaces before inline comment |
![]() |
|
The first line of this file should be: from __future__ import unicode_literals |
|
|
Can you put these into alphabetical order? |
|
|
Undo this change. |
|
|
The first line of this file should be: from __future__ import unicode_literals |
|
|
Undo this change. django, djblets, and reviewboard are all third party imports with repsect to rb-extension-pack. |
|
|
Docstrings should be of the following form: """Single line summary Multi-line description. """ |
|
|
If we're going to put a code example in the docstring, we should be using a restructured text code block. |
|
|
"A JSON blob" |
|
|
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. |
|
|
Same comment about six.text_type vs str applies here. |
|
|
You don't need pass if the class has a docstring. |
|
|
Same comment here about unicode_literals. |
|
|
Same comment about unicode_literals as above. |
|
|
Blank line between these. |
|
|
Shouldn't this be int ? The model indicates that ids are integers. |
|
|
Missing trailing comma. |
|
|
This should be six.text_type. |
|
|
Missing trailing comma. |
|
|
Missing trailing comma. |
|
|
Missing trailing comma. |
|
|
Can you catch CheckListItem.DoesNotExist here instead? |
|
|
six.text_type, not str |
|
|
six.text_type |
|
|
Can you catch CheckListItem.DoesNotExist here instead? |
|
|
six.text_type |
|
|
Should have a trailing comma. |
|
|
Trailing comma |
|
|
Trailing comma. |
|
|
Can you catch CheckListItem.DoesNotExist here instead? |
|
|
You can add checklist_item_id as a paramter to the function and just refer to it that way. Its cleaner than … |
|
|
This shoudl have from __future__ import unicode_literals |
|
|
"ID" |
|
|
Is this left over from debugging? |
|
|
This should be formatted the same as a Python docstring. This applies to all methods. |
|
|
Can we just use response.checklist_item.keys() here? |
|
|
I feel like since we're using this all over the file, and we always have access to the Checklist object, … |
|
|
"The" |
|
|
review_request_id shouldn't be quoted. |
|
|
How about _addItemToView ? |
|
|
I think it would be ok to include the keys we expect in there (id, checked, description ?). |
|
|
Won't this start indexing from 1? Shouldn't we index from 0? |
|
|
I don't believe you should be using a dummy model for the web API. Models are not required. |
|
|
This will end up in public-facing documentation. |
|
|
We don't really need this. |
|
|
Can we put a doc comment on this? |
|
|
Doc comments should be of the form /* * Single line summary. * * Multi-line description. */ Same elsewhere. |
|
|
Needs a doc comment. |
|
|
Needs long-form doc comment. |
|
|
It technically isn't a dictionary -- its a JS object. You could call it a mapping, instead. |
|
|
Needs doc comment. |
|
|
Needs doc comment. |
|
|
Should use long form doc comment. Same elsewhere. |
|
|
If you are referring to Python data types, this is actually a six.text_type. |
|
|
Same here. |
|
|
Can you fix up this docstring? |
|
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