flake8
-
reviewboard/diffviewer/commit_utils.py (Diff revision 1) Show all issues -
-
Review Request #10136 — Created Sept. 10, 2018 and submitted
This patch builds upon the UI in /r/10132/ and implements the
functionality to render the actual content of the diff specified by the
selected commit range.
Description | From | Last Updated |
---|---|---|
Would it be possible to split out the ES6 port part of this change into a separate review request? |
|
|
W391 blank line at end of file |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
W391 blank line at end of file |
![]() |
|
F841 local variable 'diffset_commits' is assigned to but never used |
![]() |
|
Col: 10 Missing semicolon. |
![]() |
|
Col: 70 Missing semicolon. |
![]() |
|
Col: 69 Missing semicolon. |
![]() |
|
request -> requested? |
|
|
request -> requested? |
|
|
There's a lot of nesting and conditionals in here. Can we split this up to be more readable? |
|
|
Too many blank lines now. |
|
|
Needs a trailing comma. |
|
|
Needs a trailing comma. |
|
|
Missing the explanations here. |
|
|
Needs a doc comment. |
|
|
Looks to me like this returns a function, not an object. |
|
|
Can we use a ~ for each class reference to trim the output a bit? |
|
|
Let's get rid of the double loop and need for constructing generators. What do you think of: needs_base_commit = base_commit_id … |
|
|
Too many blank lines. |
|
|
There's some redundancy in handling of missing data here. We know index is in GET, but we call .get() on … |
|
|
string |
|
|
string |
|
|
Can you put parens around the expression? |
|
|
These are indented too far. |
|
|
These should all align. |
|
reviewboard/diffviewer/commit_utils.py (Diff revision 1) |
---|
Rebased. Added js tests.
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+1130 -236) |
reviewboard/diffviewer/views.py (Diff revision 2) |
---|
F841 local variable 'diffset_commits' is assigned to but never used
jshint,pep8
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+1127 -236) |
reviewboard/diffviewer/commit_utils.py (Diff revision 3) |
---|
There's a lot of nesting and conditionals in here.
Can we split this up to be more readable?
reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js (Diff revision 3) |
---|
Needs a trailing comma.
reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js (Diff revision 3) |
---|
Needs a trailing comma.
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3) |
---|
Missing the explanations here.
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3) |
---|
Needs a doc comment.
Depends On: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 4 (+946 -132) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+967 -132) |
Adds missing unit tests
Depends On: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 6 (+990 -132) |
reviewboard/diffviewer/commit_utils.py (Diff revision 6) |
---|
Can we use a
~
for each class reference to trim the output a bit?
reviewboard/diffviewer/commit_utils.py (Diff revision 6) |
---|
Let's get rid of the double loop and need for constructing generators. What do you think of:
needs_base_commit = base_commit_id is not None needs_tip_commit = tip_commit_id is not None if needs_base_commit or needs_tip_commit: for commit in commits: if needs_base_commit and commit.pk == base_commit_id: base_commit = commit needs_base_commit = False if needs_tip_commit and commit.pk == tip_commit_id: tip_commit = commit needs_tip_commit = False if not needs_base_commit and not needs_tip_commit: break
reviewboard/diffviewer/views.py (Diff revision 6) |
---|
There's some redundancy in handling of missing data here. We know
index
is inGET
, but we call.get()
on it anyways. We're also then doing exception handling anywya. How about instead we do:try: diff_file['index'] = int(self.request.GET['index']) except (KeyError, ValueError): pass
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 6) |
---|
Can you put parens around the expression?
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+958 -106) |
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 7) |
---|
These should all align.