I just have a few comments, which hopefully answer some of your questions.

 - You're going to have to package the javascript files with Review Bot. This is done
   in setup.py, using 'package_data' and 'data_files' arguments to setup().

 - I think reading the file using the js engine is fine. If we were to try and pass a
   string, we'd have to worry about escaping things etc.

 - Maybe move the js files to something like 'ReviewBot/bot/reviewbot/lib/JSLint/*'

This should probably be renamed. JSLint is more than a style checker.

This should probably be adjusted like 'name'. It might be a good idea to include the version number of JSLint (Not the self.version, but the actual JSLint projects version) in the description here.

This should probably be changed to be case insensitive. I've seen some use '.JS'

I really would like to add better support for file matching to review bot in general, and make it so tool devs don't have to worry about this. I haven't thought of a good solution yet though.

This appears to still be just copied from the pep8 tool, but you'll need to change this parsing code to match the output from your javscript.

It's probably worth figuring out a way to pass these options in from the python code. If you can't pass arguments into the JS, you could probably generate 'options' in the python, and pass it as a string as part of the call to execute.

Once you solve that problem, they can be added to the admin panel like the pep8 tool's 'max_line_length'.

let's put these in alphabetical order.

Import 'os' before the other 2 modules.

Add an extra line before the class declaration (should be a total of 2 blank lines)

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

Remove trailing comma

I think this can be DRYed up a bit. Maybe we can do something like this:

settings_keys = ['debug', 'devel', 'es5', 'evil', 'passfail', 'sloppy', 'white']
jsLintOptions = dict((s, str(self.settings[s]).lower()) for s in settings_keys)
jsLintOptions['maxlen'] = int(self.settings['maxlen'])

As Aamir pointed out, 'import os' should be coming first.

Imports should be ordered like this:

Standard Library imports (e.g. os, sys, etc.)
===BLANK LINE===
Third Party imports
===BLANK LINE===
Local Imports (e.g. 'reviewbot...')

Also, imports should be ordered alphabetically within the three categories.

If this is enabled, I think you should definitely indicate in
the review body_top, or body_bottom that the scan was stopped after a
single error. Maybe even print PASSED, or FAILED etc.

In most cases we'll probably want to find all errors and print them,
so I think you should probably default this to False. I think you were
probably emulating the defaults from JSLint itself, but False seems
more natural in our setting.

Also, this reminds me, does JSLint stop on certain errors, and refuse to
continue scanning until they are fixed? If this is the case, is there a
way we can detect this, and indicate to the user that scanning stopped
after that point?

This should probably be done only once per request, instead of every
time handle_file is called (Once per file).

You'll probably want to override handle_files, or execute, and take
care of this stuff there. Make sure to call the base class' method
from your overridden method.

What exactly does JSLint output which causes the failure? I'm
wondering if it's something that should be displayed in the review
another way?

Quite the opinionated option, with quite a useless description.

Not sure what this does, but it gives me the vibe it could be a hot
button issue. Do you think this is an option we should include? I'd
say we should at least figure out what it does.

I don't think this is quite correct. The package key should probably be
'reviewbot', or 'reviewbot.tools'.

I'm fine with lib being inside the tools folder, but I think the JSLint
specific stuff needs to be put in a sub folder as well (We may end up
expanding with other things in here, and I'd like to have it organized).

So, you'll have something like:

package_data={
    'reviewbot.tools': [
        'lib/jslint/*.js',
    ]
}

Let's rename jsLint.py to jslint.py.

So, this should be changed to 'jslint = reviewbot.tools.jslint:JSLintTool',

Somme comments to guide reviewers and sollicit feedback.

Might not be necessary to sent the lowercase form of the strings. runJSLint is checking the content of the string anyway, it could check for upper case "True" and "False".

self.jsLintOptions is a dict. wondering if we should depend on auto-formatting of dict to string used here or whether we should use an explicit json.dumps() type conversion to string. Thoughts on why that might be advantageous?

See comment on line 127 of jslint.py. Originally this check did not exist, and we passed the straight string value to jslint as an option. Since conversion to boolean value, kept on accepting lower case string since it best mimics the js boolean values (you could argue in the interest of modularity, if another program were to use this js script, they might not send in uppercase 'True' and 'False', so we are picking a standard).

Just a few comments from reading over the code

Should this be called whitespace instead?

Maybe you could add a comment about which file caused the failure?

I believe, generally, we prefer the higher level imports to go above the more granular ones, so line 3 and 4 should be swapped.

Evil? Or Eval? Or Evil-Eval?

Not sure if this is being cheeky, or just a typo. :)

Isn't 80 the traditional character limit? Or is something here 0-indexed? Or have I just been lied to all my life?

PACKAGE_ROOT is in all caps, and lib_path is not. Why?

Can you not simply pass the actual True and False literals to the script? Will the JS engine accept something like:

js -e "load('foo'); runJSLint('bar', read('baz'), {someOption: false, otherOption: true}"

?

Yeah, maybe this is overkill then.

This line will need to be indented two more spaces, since it's still inside the dicts parens.

Is there a reason this is not called *before* we do all of JSLineTool's specialized stuff?

Let's put a newline after the if-block.

I don't think the string representation of dict for the things you're passing is going to change anytime soon, so I'm not too worried, personally.

This line needs to have two spaces removed from its indentation.

Yeah, a little less silent-fail would probably be good here.

This function should be documented.

Also, as a general rule, we tend to use 4-space indentation for Javascript.

var options

Should be formatted like:

if (custom_options) {
  ...
}

for (var op in custom_options) {

var custom

It'd be nice if the caller could just pass in things typed as one would expect, without having to jump through type conversion hoops...see my question above about passing boolean literals.

It'd be awesome to have the tool return an all-clear. Maybe even a ship-it, if the tool is configured for it. (The ship-it thing might be out of scope, and a nice-to-have...but I think hearing about a no-error run would be a good thing).

} else {

Spaces on either side of the =, space before the {

Space after the if and before the {

Spaces on either side of the +'s, and line up line 33 so that it's still contained (vertically) within the parentheses of the above line.

The latest diff seems to have been uploaded strangely, only showing part of the change. Could you please update this again with the full patch?

Allyshia, this is looking really good. I pulled the patch down, and
ran Review Bot locally against it with PEP8 and JSLint :D

We are going to have to update this to be a separate package before
it can be committed though (The JSLint Licensing issues :/). We can
discuss that in IRC.

2 Blank lines here.

> 79 characters

Can we break this out one per line, 4 space indented.

I'm curious why you're having to call int(...) here. The values of self.settings *should* be the proper type, is this not the case?

Could we change the formatting here a bit (Also note the trailling comma):

"load('%s'); runJSLint('%s', read('%s'), %s);" % (
    self.runJSLint_path,
    self.jsLint_path,
    path,
    json.dumps(self.jsLintOptions)),

Since this is being continued in a different review request, can we mark this as discarded?