Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/lib/js/ui.autocomplete.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/search.py

Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/search.py

Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/lib/js/ui.autocomplete.js

One comment I've seen repeated is that our request description should kind've "tell a story" with respect to the project. It'd be useful to me (and maybe other students) to hear about the purpose of this project and how your changes accomplish that.

This is a good time to ask, if we can leave comments in a request. I think they should be removed so as they are not forgotten and published with it

this can use the operator "===" for extra security

if changing the operator to "===" is okay with you

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

Can you rewrite your summary in the imperitive mood? i.e, "Add auto-complete functionality to the Depends On field"

Can you add screenshots?

Fix indentation.

Can you remove the spacing between the parentheses?

Undo this change.

This can be a one line comment. I'm not sure if its necessary, though.

Undo this.

Is this intentional?

space after colon

Undo this.

Undo this.

Undo this.

Undo this.

Remove this line.

Undo this.

Undo this.

Remove this line.

Is this still WIP

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

This file is all third-party code. I don't think we want to make any style fixes in here, and even if we did, they should be in a separate change so they don't obscure the few changes you did make to the code.
That said, I don't think any changes should be required in here. If they are, we definitely need to look at them closely.

It's probably a little silly to define two new variables when these are only used once. In fact, this whole function could be:

cmp: function(term, a, b) {
   return b.data.id - a.data.id;
}

Please undo this change.

What are these changes?

It's okay to explicitly compare against true in cases where this could take other falsy values, but we should at least use !== instead of != because != will implicitly cast, which makes the explicit comparison moot.

The var statement should be the first thing in the function body.

In es5 code (which this is, since we haven't yet ported it to es6), the var statement should be the first thing in the function. That means we need to postpone the assignment of a few of these:

var parsed = [],
    items,
    value,
    i;

if (options.resourceName === 'search') {
    data = data.search;
}

items = data[options.fieldName];

for (i = 0; i < items.length; i++) {
    ...
}


Note that I've removed the kind of useless itemsLen variable. I'd also like the "search" special case to be commented explaining why it's there.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

There's still a bunch of style changes in this file which are obscuring your functional changes. Please go through until the diff has only the changes which actually affect the behavior, and then we can discuss why they're necessary.

Multi-line comments should use /* ... */.

Trailing whitespace.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

Can you explain why you need all these new String() calls?

Braces.

Prefer:


var data;

if (!options.matchCase && typeof term === 'string') {
    term = term.toLowerCase();
}

data = cache.load(term);


We want to use === basically always.

!==, 'string'

blank line.

This needn't be two conditions:

if (options.fieldName === 'review_requests' && !data.public) {
    return false;
}

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

Good job so far. My biggest concern is that there are changes made that are too global.
I'm not sure you need to make any modifications to ui.autocomplete.js. Instead, the code working with the autocomplete widget should be responsible for returning the appropriate data types. By casting to a string, you can alter behavior that might conceivably exist in other use cases, and make it harder to eventually move away from this widget (as the data type casting behavior is "hidden" from the caller's code).
I also have some suggestions for the if checks in the autocomplete invocation in reviewRequestEditorView.js, intended to keep from adding field-specific behavior here (fields are pluggable and we don't want to hard-code things if we can help it).

I'm not sure I'm comfortable with this here. The code using autocomplete should be the one responsible for supplying strings.

Similarly here. I don't think we should be enforcing strings here.

No space after function.

A couple of stylistic comments:
* Must be 4 space indentation.
* The if line is too long. Lines should not exceed 79 characters. This should wrap after &&.
The bigger thing is that this begins a precedent of field-specific logic in here. We don't want that. Fields are pluggable, and we want extensions to be able to provide their own fields.
In this particular case, what we're saying is "hide any private review requests." We shouldn't even be getting these in the result to begin with. What you instead need to do is modify the API request to exclude any private review requests for this field, so they never come through to begin with. We can then safely display anything that we get back.
This is very important. Imagine if we only get 20 results back and the first 18 are private. That'd mean we only get 2 public review requests to display, instead of 20. That'd be a confusing experience.

Variables with values should come before variables without.

We don't want to have field-specific logic here. Instead, we want the code registering a field to be able to parse the result.
If you look at registerField, you'll see where fields get registered. There's an option (which sadly is not documented there, but you can see usage in setupFieldEditor) that indicates if autocomplete behavior should be used. This is a dictionary of things like fieldName, nameKey, etc.
What I'd recommend is adding another optional thing in that dictionary: parseResultList. This would be a function that takes the data and return the list of items. If that exists in the dictionary, it'd be called, and if not, the default items = data[options.fieldName] would be used.
Then the Depends On handler could provide its own implementation, removing the hard-coded support for data.search.

No space after function.

Instead of $($field.children()[0]), you can do $field.children().eq(0), etc.
Also, we should fetch $field.children() only once.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

A few things here.
First, javascript variables should be camelCase.
Secondly, combine this into one var statement:

var unwrappedData = data.search[options.fieldName],
    i;


(because of var hoisting)

We always use /* block comments */.

['public'] as public is a reserved word and will break in IE.

i--

Reformat as:

unwrappedData[i].id = unwrappedData[i]
    .id
    .toString();

No trailing comma (will break IE)

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

This function can be written much more simply:


function(data, options) {
    return data.search[options.fieldName]
        .filter(function(datum) { return !datum['public']; })
        .map(function(datum) {
            datum.id = datum.id.toString();
            return datum;
        });
});

if (!unwrappedData[i]['public']) {

Shouldn't this be return unwrappedData?

We don't need to add this. We should be setting display_name for the results instead in the caller.

Space after :

We should never have private reviews here, under any circumstances. The API must return only public results.

We access unwrappedData[i] a lot. let's pull that out once.

Once unwrappedData[i] is pulled out, this can/should be one line.

Indentation is still wrong.

We shouldn't be touching this code. Only the new field should be modified.

Same here.

You got rid of itemsLen. There's no way this code works. Please make sure this is all very properly tested.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/lib/js/ui.autocomplete.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Can we pull out unwrappedData[i] into another variable, say datum and use that here?

Can we call it item instead of value?

I don't think this extra level of nesting is required. You're not adding beforeEach or afterEach.

Indentation is off by 1 space for the for loop here.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Looking at the code now, I think we can make a major simplification.
Right now, we have two iterations over the list of items--one in parseResultList, and one in parse. A simpler approach would be to have an optional parseItem method which would modify an individual item in the results, looking something like:

parseItem: function(item) {
    item.id = item.id.toString();
    item.display_name = item.summary;
    return item;
}


Inside parse, you'd then do something like:

for (i = 0; i < items.length; i++) {
    item = items[i];

    if (options.parseItem) {
        item = options.parseItem(item);
    }

    parsed.push({
        ...
    });
}

Your indentation is still funky here (the for loop and everything in it needs to be indented 1 more space).

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

One nice way to clean this up would be to make it so options.fieldName could be either a function or an actual field name. This would then be:

if (_.isFunction(options.fieldName)) {
    items = options.fieldName(data);
} else {
    items = data[options.fieldName];
}


and the fieldName for the depends_on field would be:

fieldName: function(data) {
    return data.search.review_requests;
}


That way we don't have to have both fieldName and the new formatData function.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Ship It!