Adding JSLint to ReviewBot

Review Request #3601 — Created Dec. 1, 2012 and submitted

Information

reviewbot-jslint

Reviewers

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.
- posted multiple review requests locally, with js files and non js files, and ensured reviews were generated, with appropriate comments.
- 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).
- tested changing options in the option panel, and made sure they affected 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

I'll quote PEP8 again: "Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be …

SM smacleod

Comments should be a proper sentence, ending with a period.

SM smacleod

The upercase variable names give me the impression they are globals. Let's lowercase these.

SM smacleod

lets lower case these as well

SM smacleod

After the changes suggested below, this should be readable enough without the comments.

SM smacleod

Now that you aren't calling int(...) on the numeric keys, how about we combine these. Also, can you rename this …

SM smacleod

lets lower case 'jslint_options'

SM smacleod

I think we can axe this comment, the code itself should be explanatory.

SM smacleod

Can you put a blank line between these (RB convention to put a blank line after indented blocks like conditionals …

SM smacleod

I think we can axe this comment.

SM smacleod

I think it's sufficient to go with something like 'ReviewBotJSLint'

SM smacleod

Somewhere in here you need to add: install_requires=[ 'reviewbot', ] I'd stick it after entry_points

SM smacleod

I would change this description to say that it's a Review Bot tool which runs JSLint. This is actually a …

SM smacleod

I'll quote PEP8 for this one: "Modules should have short, all-lowercase names. Underscores can be used in the module name …

SM smacleod

These two variables are still uppercase. Once they are lower cased, I'll commit this.

SM smacleod
SM
  1. This is really close, just a few style things, and some packaging issues. Looks good :D
  2. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
     
    Show all issues
    I'll quote PEP8 again:
    "Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation."
    
    Lets change this to:
    
    description = ("Checks syntax errors and validates JavaScript using the "
                   "JSLint tool."
  3. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
    Show all issues
    Comments should be a proper sentence, ending with a period.
  4. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
     
    Show all issues
    The upercase variable names give me the impression they are globals. Let's lowercase these.
  5. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
     
    Show all issues
    lets lower case these as well
  6. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
    Show all issues
    After the changes suggested below, this should be readable enough without the comments.
  7. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Now that you aren't calling int(...) on the numeric keys, how about we combine these.
    
    Also, can you rename this 'jslint_option_keys' and drop the ']' to its own line.
  8. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
    Show all issues
    lets lower case 'jslint_options'
  9. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
    Show all issues
    I think we can axe this comment, the code itself should be explanatory.
  10. reviewbotJSLintTool/jslint.py (Diff revision 1)
     
     
     
    Show all issues
    Can you put a blank line between these (RB convention to put a blank line after indented blocks like conditionals and loops)
  11. reviewbotJSLintTool/lib/runJSLint.js (Diff revision 1)
     
     
    Show all issues
    I think we can axe this comment.
  12. setup.py (Diff revision 1)
     
     
    Show all issues
    I think it's sufficient to go with something like 'ReviewBotJSLint'
  13. setup.py (Diff revision 1)
     
     
    Show all issues
    Somewhere in here you need to add:
    
    install_requires=[
        'reviewbot',
    ]
    
    I'd stick it after entry_points
  14. setup.py (Diff revision 1)
     
     
    Show all issues
    I would change this description to say that it's a Review Bot tool which runs JSLint. This is actually a description which will be used when this is packaged and distributed as an egg.
  15. setup.py (Diff revision 1)
     
     
    Show all issues
    I'll quote PEP8 for this one:
    
    "Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged."
    
    with that being said, how about we go with something like 'reviewbotjslint' as the package name.
    
    You'll need to rename the reviewbotJSLintTool folder to 'reviewbotjslint' as well.
  16. 
      
AL
SM
  1. Very close, just one little nit.
  2. reviewbotjslint/jslint.py (Diff revision 2)
     
     
     
    Show all issues
    These two variables are still uppercase. Once they are lower cased, I'll commit this.
  3. 
      
AL
SM
  1. Ship It!
  2. 
      
AL
Review request changed
Status:
Completed
Change Summary:
Pushed to master at https://github.com/smacleod/reviewbot-jslint (4392e2ce8f).