- Change Summary:
-
Add comments and documentation. And yay to fixing of the parent-diff bug!
- Summary:
-
[WIP] [checklist] Refactoring of checklist[checklist] Refactoring of checklist
- Description:
-
Refactoring includes
~ - Use of
request.user
instead of passinguser.id
in every HTTP request
~ - Renamed status of each item from
finished
tochecked
~ - Use of
ChecklistItem
non-database resource instead of based on HTTP request parameters
~ - Moving of front-end API logic from view to model
~ - CSS hyphens everywhere instead of underscores
~ ~ Why is this a [WIP]?
~ - Use of
request.user
instead of passinguser_id
in every HTTP request
~ - Updating of the view only after successful API response
~ - Renamed status of each item from
finished
tochecked
~ - 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)
- There's an error when viewing the diff of one the files in this review request. - Django back-end code got slightly heavier, so aiming to lighten it a bit before finalizing the change. - This may very well not be useful for checklist templates integration, but we'll see. - Use of
- Testing Done:
-
~ Basic CRUD functionality remains.
~ Basic CRUD functionality remains. Ensured that the views will not update unless there is a successful response from the server.
- Diff:
-
Revision 2 (+417 -337)
[checklist] Refactoring of checklist
Review Request #7176 — Created April 7, 2015 and submitted
Refactoring includes
- Use of
request.user
instead of passinguser_id
in every HTTP request - Updating of the view only after successful API response
- Renamed status of each item from
finished
tochecked
- 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 |
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 |
- Change Summary:
-
Fix ReviewBot issues.
- Diff:
-
Revision 3 (+415 -336)
-
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>
-
-
-
-
-
Undo this change.
django
,djblets
, andreviewboard
are all third party imports with repsect torb-extension-pack
. -
-
If we're going to put a code example in the docstring, we should be using a restructured text code block.
-
-
This should probably be
six.text_type
(from django.utils.six
), notstr
. If this should be a byte string type, usesix.binary_type
instead. -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
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. -
-
-
-
-
-
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. -
-
-
- Change Summary:
-
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
- Change Summary:
-
Use
six.text_type
instead ofstr
.
Use reStructuredText in comment block.
Improvements for comments, variables and Python method arguments. - Commit:
-
74ba2ebb4e3ac643db885d2a56d1fe5928d7dd72d7e1a99f7c04c5abdd3c6b8fb0d7373684e8dc18
- 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
- Change Summary:
-
Update comments and docs.
Checklist item indices now starts from 0 instead of 1. - Description:
-
Refactoring includes
- Use of
request.user
instead of passinguser_id
in every HTTP request
- Updating of the view only after successful API response
- Renamed status of each item from
finished
tochecked
- 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.
- Use of
- Commit:
-
d7e1a99f7c04c5abdd3c6b8fb0d7373684e8dc18a4bc78a7853d6e01dcf148b12ed281700a756a2a
- 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
- Change Summary:
-
Six is lucky seven ate nine.
- Commit:
-
a4bc78a7853d6e01dcf148b12ed281700a756a2a6fdc85454decac337d0e8b76ff877441eede6da8
- 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
- Change Summary:
-
Fix docstring.
- Commit:
-
6fdc85454decac337d0e8b76ff877441eede6da821244921718869a60cc09123714ad76bd2e1a406
- 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