Adding JSLint to ReviewBot

Review Request #3435 — Created Oct. 21, 2012 and discarded

Allyshia
ReviewBot
reviewbot
Implementation of the JSLint tool for ReviewBot. Parses all uploaded files to a review request and outputs descriptive comments on the relevant .js source files on lines where style errors are found. Options for this tool can be configured in the tool's admin panel. This addition to ReviewBot requires that SpiderMonkey be installed (other engines might also be suitable but have not been tested) to enable the running of the jslint.js linting tool.
manual testing:
- tested jslint on the spidermonkey commandline to make sure that the script itself was working as expected locally.
- tested what error output the jslint.com website provides for a given file and tested ReviewBot on local dev server tool by posting a review with same file and ensuring output is the same with the same options ticked (seeing that file with errors was being commented by ReviewBot).
- started by importing all of the dependencies used by the jslint.com website and removed all which were not necessary to the standalone jslint tool, testing that it yielded the same results. Those other dependencies were only necessary for web purposes, even if Doug Crockford suggests they be bundled with jslint itself and minified.
- tested changing options in the option panel, and making sure they affect the output of the static analysis tool in locally published reviews. Included print statements in py and js scripts to make sure the correct options were being passed into jslint.
Description From Last Updated

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

SM smacleod

This should probably be adjusted like 'name'. It might be a good idea to include the version number of JSLint ...

SM smacleod

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

SM smacleod

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

SM smacleod

let's put these in alphabetical order.

SM smacleod

As Aamir pointed out, 'import os' should be coming first. Imports should be ordered like this: Standard Library imports (e.g. ...

SM smacleod

If this is enabled, I think you should definitely indicate in the review body_top, or body_bottom that the scan was ...

SM smacleod

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

SM smacleod

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

SM smacleod

Let's rename jsLint.py to jslint.py. So, this should be changed to 'jslint = reviewbot.tools.jslint:JSLintTool',

SM smacleod

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

mike_conleymike_conley

Evil? Or Eval? Or Evil-Eval? Not sure if this is being cheeky, or just a typo. :)

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

Might not be necessary to sent the lowercase form of the strings. runJSLint is checking the content of the string ...

AL Allyshia

Yeah, maybe this is overkill then.

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

self.jsLintOptions is a dict. wondering if we should depend on auto-formatting of dict to string used here or whether we ...

AL Allyshia

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

mike_conleymike_conley

This function should be documented. Also, as a general rule, we tend to use 4-space indentation for Javascript.

mike_conleymike_conley

var options

mike_conleymike_conley

Should be formatted like: if (custom_options) { ... }

mike_conleymike_conley

for (var op in custom_options) {

mike_conleymike_conley

var custom

mike_conleymike_conley

See comment on line 127 of jslint.py. Originally this check did not exist, and we passed the straight string value ...

AL Allyshia

It'd be nice if the caller could just pass in things typed as one would expect, without having to jump ...

mike_conleymike_conley

It'd be awesome to have the tool return an all-clear. Maybe even a ship-it, if the tool is configured for ...

mike_conleymike_conley

} else {

mike_conleymike_conley

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

mike_conleymike_conley

Space after the if and before the {

mike_conleymike_conley

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

mike_conleymike_conley

2 Blank lines here.

SM smacleod

> 79 characters

SM smacleod

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

SM smacleod

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

SM smacleod

Could we change the formatting here a bit (Also note the trailling comma): "load('%s'); runJSLint('%s', read('%s'), %s);" % ( self.runJSLint_path, ...

SM smacleod
AL
AL
SM
  1. 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/*'
  2. bot/reviewbot/tools/jsLint.py (Diff revision 2)
     
     
    This should probably be renamed. JSLint is more than a style checker.
  3. bot/reviewbot/tools/jsLint.py (Diff revision 2)
     
     
    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.
  4. bot/reviewbot/tools/jsLint.py (Diff revision 2)
     
     
    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.
  5. bot/reviewbot/tools/jsLint.py (Diff revision 2)
     
     
     
     
     
     
    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.
  6. bot/reviewbot/tools/runJSLint.js (Diff revision 2)
     
     
     
     
     
     
    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'.
  7. bot/setup.py (Diff revision 2)
     
     
     
    let's put these in alphabetical order.
  8. 
      
AL
AL
AL
  1. 
      
  2. 
      
AA
  1. 
      
  2. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Import 'os' before the other 2 modules.
  3. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Add an extra line before the class declaration (should be a total of 2 blank lines)
  4. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
    1. I would actually suggest leaving the trailing commas.
      
      http://docs.python.org/faq/design.html#why-does-python-allow-commas-at-the-end-of-lists-and-tuples
    2. Steven:
      
      Gotcha. I suggested removing them because I had to remove a couple on r/3434. Thanks!
  5. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  6. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  7. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  8. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  9. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  10. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  11. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  12. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  13. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  14. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  15. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  16. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  17. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  18. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  19. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    Remove trailing comma
  20. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
    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'])
  21. 
      
SM
  1. 
      
  2. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
     
     
    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.
  3. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
     
     
    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?
    1. JSLint will stop after a certain threshold and refuses to continue scanning. (This is why some people prefer JSHint). At that point, there is an inline comment that indicates that scanning was stopped. Ideally (and it appears consistently in my trials on jslint.com) that the cut off would end up being around the 70-80% range of scanning on average. The passfail setting being false should ensure that the most scanning possible will happen. I will default to false as you suggest. For the cutoff point though, is an inline comment enough to indicate to the user that scanning stopped?
  4. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.
  5. bot/reviewbot/tools/jsLint.py (Diff revision 3)
     
     
     
    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?
    1. Actually I am not sure this except is still justified. A while back I was having runJSLint.js output "no errors" if no errors were found, so at this point in jsLint.py, there would be no correctly formatted line/col numbers. JSLint shouldnt really output anything that isnt a line/col/message, but I'd have to verify. Is it worth leaving the except for safety ?
  6. bot/reviewbot/tools/jslint.js (Diff revision 3)
     
     
    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.
    1. Yeah it really is vague. I would be interested to see what this option does, and I don't think we should offer up this option to RB users in the options panel (at least not by this name) unless it really proves to do something useful.
    2. Just to follow up, this option finds uses of blocking "Sync" methods in Node.Js code. Enabling the "tolerate stupid" option allows these instances of "Sync" methods, which generally would be a matter of taste / design decisions. So I don't think the RB users should necessarily see this option in their panel, and we should leave the JSLint default at true, since forbidding Sync can be controlled by human discretion.
  7. bot/setup.py (Diff revision 3)
     
     
    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',
        ]
    }
    1. Sounds good. Will do that.
  8. bot/setup.py (Diff revision 3)
     
     
    Let's rename jsLint.py to jslint.py.
    
    So, this should be changed to 'jslint = reviewbot.tools.jslint:JSLintTool',
  9. 
      
AL
AL
  1. 
      
  2. 
      
AL
  1. Somme comments to guide reviewers and sollicit feedback.
  2. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    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".
  3. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    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?
    1. See my comments on Mike's review. We want to use json.dumps()
  4. 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).
    1. This will be fixed by other changes, see comments on Mike's review.
  5. 
      
TA
  1. Just a few comments from reading over the code
  2. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    Should this be called whitespace instead?
  3. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    Maybe you could add a comment about which file caused the failure?
  4. 
      
mike_conley
  1. 
      
  2. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
     
    I believe, generally, we prefer the higher level imports to go above the more granular ones, so line 3 and 4 should be swapped.
    1. Oops - I also copied this from pep8.py -- should both be changed?
    2. It should be changed here, but don't worry about for pep8. There
      are a number of style problems with the pep8 tool, and I'm going
      to fix them along with some other modifications I'm making to it.
  3. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    Evil? Or Eval? Or Evil-Eval?
    
    Not sure if this is being cheeky, or just a typo. :)
    1. I should probably state that these variable names were borrowed straight from JSLINT's own nomenclature so as to establish a relationship between our options and the JSLINT options. If this is not a good idea, please let me know.
    2. I would say keep the relationship with the JSLint options.
  4. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    Isn't 80 the traditional character limit? Or is something here 0-indexed? Or have I just been lied to all my life?
    1. I actually borrowed this from our PEP8 tool's default. Should that be 80...?
    2. The preferred maximum line length for Python, according to PEP8, is 79
      characters: http://www.python.org/dev/peps/pep-0008/#maximum-line-length
      
      As for the JSLint default here, Crockford says, "Avoid lines longer than 80
      characters". So we should probably bump this to 80.
  5. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    PACKAGE_ROOT is in all caps, and lib_path is not. Why?
  6. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    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}"
    
    ?
    1. Since all of the arguments passed in to the jslint.js script via the spidermonkey command line are done here by string value, True ends up as 'True', not as its boolean equivalent. Hence the need to translate at some point between the uppercase-led string and the boolean in js. (which speaks to the reviewer's comments on line 11 of runJSLint.js). Is this acceptable?
    2. So, the problem is stemming from the fact that we're passing the options
      dictionary as a string like this:
      
      148  "load('%s'); runJSLint('%s', read('%s'), %s);"
      149      % (self.runJSLint_path, self.jsLint_path, path,	
      150         self.jsLintOptions)
      
      If you take this dictionary, with the python True & False, and run it
      through 'json.dumps()', you should be able to pass it in, and have
      proper JS true and false.
  7. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    Yeah, maybe this is overkill then.
  8. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    This line will need to be indented two more spaces, since it's still inside the dicts parens.
  9. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    Is there a reason this is not called *before* we do all of JSLineTool's specialized stuff?
    1. If I call super(...) first, that will fire off the call to handle_file(...) (handle individual file) for each file in the review request (also overridden in JSLintTool). The variable initialization that happens in the beginning of the handle_files(...) method overridden in JSLintTool must necessarily occur before that happens.
  10. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    Let's put a newline after the if-block.
  11. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
    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.
  12. bot/reviewbot/tools/jslint.py (Diff revision 4)
     
     
     
     
     
     
    Yeah, a little less silent-fail would probably be good here.
    1. A couple of options here:
      1- return False and count this file as ignored rather than processed. The list of ignored and processed files are displayed to the user in the body_top of the review.
      2- explicitly tell the user that an error occurred while processing the file, i.e. in an ERROR section of the review. No support for this yet.
      
      In general this use case should occur rarely.
      
      Thoughts on this would be appreciated.
    2. Since we don't have the proper support for errors, just add to the ignored list
      for now. Please put a TODO comment indicating we should change this to properly
      display an error message.
  13. This function should be documented.
    
    Also, as a general rule, we tend to use 4-space indentation for Javascript.
  14. var options
  15. Should be formatted like:
    
    if (custom_options) {
      ...
    }
  16. for (var op in custom_options) {
  17. 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.
    1. As mentioned above, passing the dictionary in JSON encoded should fix this.
  18. bot/reviewbot/tools/lib/jslint/runJSLint.js (Diff revision 4)
     
     
     
     
    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).
    1. Review Bot already contains support for Ship-It on no issues. Right now
      no errors is just a review telling you the processed files and having no
      comments, but a nice message could be useful.
  19. bot/reviewbot/tools/lib/jslint/runJSLint.js (Diff revision 4)
     
     
     
    } else {
  20. Spaces on either side of the =, space before the {
  21. Space after the if and before the {
  22. bot/reviewbot/tools/lib/jslint/runJSLint.js (Diff revision 4)
     
     
     
    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.
  23. 
      
AL
AL
AL
SM
  1. 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?
  2. 
      
AL
SM
  1. 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. bot/reviewbot/tools/jslint.py (Diff revision 6)
     
     
     
     
    2 Blank lines here.
  3. bot/reviewbot/tools/jslint.py (Diff revision 6)
     
     
    > 79 characters
  4. bot/reviewbot/tools/jslint.py (Diff revision 6)
     
     
     
    Can we break this out one per line, 4 space indented.
  5. bot/reviewbot/tools/jslint.py (Diff revision 6)
     
     
    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?
  6. bot/reviewbot/tools/jslint.py (Diff revision 6)
     
     
     
     
    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)),
  7. 
      
AL
mike_conley
  1. Since this is being continued in a different review request, can we mark this as discarded?
  2. 
      
AL
Review request changed

Status: Discarded

Loading...