-
-
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."
-
-
-
-
-
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.
-
-
-
Can you put a blank line between these (RB convention to put a blank line after indented blocks like conditionals and loops)
-
-
-
Somewhere in here you need to add: install_requires=[ 'reviewbot', ] I'd stick it after entry_points
-
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.
-
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.
Adding JSLint to ReviewBot
Review Request #3601 — Created Dec. 1, 2012 and submitted
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 |
- Change Summary:
-
- formatting changes, some clean-up, axing useless comments, nomenclature changes