-
-
reviewboard/reviews/models.py (Diff revision 1) This function should have a docstring saying what it does. That follows the form of:
"""One line summary Multi-line description. """
-
-
reviewboard/reviews/models.py (Diff revision 1) This is a bit complicated. I'd suggest breaking it up:
q = User.objects.filter(...) q = q.distinct().order_by(...) return q[:5]
Do you need
set()
? I would expectdistinct()
to take care of this. -
reviewboard/reviews/views.py (Diff revision 1) The other pages (for review UIs and the diff viewer) will also need this.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) _beginEditField
is too generic a name for this action. -
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Blank line after this.
Also, you need some scoping so this doesn't search the entire page. You can use
this.$('.suggest_user')
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) You should re-query here. Instead, in
render()
, store a reference to this element, and use that here. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Only one
var
statement per function, at the top, like this:var allPeople = ..., i;
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Blank line before this, and remove the trailing whitespace.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) There's some alignment and style guideline issues.
For example, you should have spaces around operators (
-
,==
, etc.)Also, in JavaScript, comparison should almost always be done with
===
and!==
(the==
and!=
versions do casting, and are slower... yes, it's weird). -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) A lot of the same comments about field references, variables, etc.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) I don't think
setEditFieldValue
is a thing. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) You can just use
show()
andhide()
instead of setting a class for elements. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) A lot of the same comments as above with variables, references, style guidelines.
This seems to me that it does the same thing as
_relayoutSuggestions
above. Am I right? If so, we should just be using that function.Also, really, this function should not know about specific field types. Instead, we should probably have some new, optional methods in the entries for
defaultFields
that are called here when editing begins (beginEdit
above) and when it ends (complete
), so that the field itself can control this UI. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Blank line before this.
Single quotes.
===
-
reviewboard/templates/reviews/review_detail.html (Diff revision 1) This line doesn't need to be removed.
-
reviewboard/templates/reviews/review_request_box.html (Diff revision 1) Some general comments:
- The HTML tags should be indented relative to their parent tag.
-
Template tags have the
{%
part unindented, and then spaces between that and the template tag name indented 1 relative to the parent template tag, like:{% if foo %}
{% for x in y %}
{% endfor %}
{% endif %}
-
reviewboard/templates/reviews/review_request_box.html (Diff revision 1) Ideally,
<br />
shouldn't be used. Instead, make the thing below in a<div>
. -
reviewboard/templates/reviews/review_request_box.html (Diff revision 1) CSS class names should use
-
instead of_
as separators.
[WIP] First attempt at front-end reviewer suggestions
Review Request #4877 — Created Nov. 2, 2013 and discarded
First attempt at the front-end for suggesting reviewers.. It works, but I think it's a bit messy in places and I'm not sure that everything is in the right place.
Clicking on a name in the suggestions adds to the target people list and pulls them off the suggestions, and hitting cancel puts them back into the suggestions.
experimented with the front-end and made sure that everything behaves as I expected:
- clicking on a suggestion when not in inline-edit mode opens the inline editor and adds the suggestion to the list, removing from suggestions list
- clicking on a suggestion when in inline-editor keeps the inline editor open and appends the suggestion, removing it from the suggestions list
- hitting cancel on the people's field throws out the added suggestions from the inline editor and adds them back to the suggestions list.
- opening another field's inline editor while the people's editor is open and has unsubmitted changes from suggestions, then hitting cancel on the other inline editor. This doesn't affect the suggestions list at all, nor the people field's current inline Edit contents. Hitting cancel on the People's inline edit field then behaves as it should (described above)
- Hitting ok on the inline editor after it has been modified by suggestions turns off inline edit mode while reflecting the changes, and keeps the suggestions list unaltered, then turning it on and hitting cancel doesn't add back to the suggestions list the suggestions that were pushed to the editor before the save.Confirmed that unit tests passed.
Description | From | Last Updated |
---|---|---|
This function should have a docstring saying what it does. That follows the form of: """One line summary Multi-line description. … |
|
|
Blank line before this. |
|
|
This is a bit complicated. I'd suggest breaking it up: q = User.objects.filter(...) q = q.distinct().order_by(...) return q[:5] Do you … |
|
|
The other pages (for review UIs and the diff viewer) will also need this. |
|
|
_beginEditField is too generic a name for this action. |
|
|
Missing a doc comment. |
|
|
Blank line after this. Also, you need some scoping so this doesn't search the entire page. You can use this.$('.suggest_user') |
|
|
You should re-query here. Instead, in render(), store a reference to this element, and use that here. |
|
|
Only one var statement per function, at the top, like this: var allPeople = ..., i; |
|
|
Blank line before this, and remove the trailing whitespace. |
|
|
There's some alignment and style guideline issues. For example, you should have spaces around operators (-, ==, etc.) Also, in … |
|
|
A lot of the same comments about field references, variables, etc. |
|
|
I don't think setEditFieldValue is a thing. |
|
|
You can just use show() and hide() instead of setting a class for elements. |
|
|
A lot of the same comments as above with variables, references, style guidelines. This seems to me that it does … |
|
|
Blank line before this. Single quotes. === |
|
|
This line doesn't need to be removed. |
|
|
Some general comments: The HTML tags should be indented relative to their parent tag. Template tags have the {% part … |
|
|
I think my update indents correctly now, but correct me if I'm wrong. |
EN endee | |
Ideally, <br /> shouldn't be used. Instead, make the thing below in a <div>. |
|
|
CSS class names should use - instead of _ as separators. |
|
|
I think this is doing a lot of unnecessary work. The .distinct() call should make the set() unnecessary, and converting … |
|
|
This looks unused? |
|
|
Did you test what happens if you click on suggestions both while the edit box is open and when it's … |
|
-
-
reviewboard/templates/reviews/review_request_box.html (Diff revision 1) I think my update indents correctly now, but correct me if I'm wrong.
-
-
reviewboard/reviews/models.py (Diff revision 2) I think this is doing a lot of unnecessary work. The .distinct() call should make the set() unnecessary, and converting to set/list will evaluate the entire queryset when all you care about is 5 objects.
QuerySet implements slicing in a way that will avoid extraneous database access, so you can just do this:
return q[:5]
-
-
reviewboard/reviews/models.py (Diff revision 2) I was doing that before, and it didn't actually give me unique values, but the use of set was just a temporary hack so I could send something that appeared sensible to the front end... I plan to redo most of this method soon.
-
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) Did you test what happens if you click on suggestions both while the edit box is open and when it's not?
I'd like to see specific test cases you executed in the testing done, instead of just "behaves as I expected"
Testing Done: |
|
---|