JSlint check#75
Conversation
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Hi @frogsandsocks, thx for participating with us!
The patch looks good 👍 , just a few minor improvements are needed.
Please squash the two commits into one and write a commit message that is in line with out commit message style (described in our contribution guidelines), then address the inline comments below.
| "dismissAddAnotherPopup", | ||
| "FileReader", | ||
| "Image", | ||
| "L", |
| - ./runisort | ||
|
|
||
| # Install NVM: | ||
| - curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.33.0/install.sh | bash |
There was a problem hiding this comment.
we did a similar tasks in other modules and this was not needed, we want to keep the build step as simple and fast as possible, therefore I kindly ask to please remove it
| - curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.33.0/install.sh | bash | ||
|
|
||
| # Install nodeJS: | ||
| - nvm install v8 |
| - python setup.py -q develop | ||
|
|
||
| # Install JSlint: | ||
| - npm install --save jslint -g |
There was a problem hiding this comment.
move this in before_install section
| - coverage run --source=django_netjsongraph runtests.py | ||
|
|
||
| # JSlint check: | ||
| - jslint --config ./.jslintrc ./django_netjsongraph/static/netjsongraph/js/*.js |
There was a problem hiding this comment.
move this in before_install section, after installing jslint
we do this to stop the build if any style check fails before installing the other python dependencies, which speeds up a lot the style checks and failures are reported very early if they happen
| - "2.7" | ||
|
|
||
| node_js: | ||
| - "8" |
There was a problem hiding this comment.
this should not be needed (we have added similar checks in other modules without adding this), try to remove it
|
@nemesisdesign |
|
@nemesisdesign |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
you were mostly done but the change in indentation was not a good idea, see below.
Please claim again the task in the GCI system so we can finish it ;-)
|
|
||
| # Install JSlint: | ||
| - npm install --save jslint -g | ||
|
|
There was a problem hiding this comment.
remove blank line, also remove the comments because they are not necessary (it's pretty obvious that we are installing jslint and we are running a jslint check), also remove the blank line after - ./runisort
| "rhino": false, | ||
| "widget": false, | ||
| "windows": false, | ||
| "indent": 2, |
There was a problem hiding this comment.
please use 4 spaces indentation and revert the changes to the JS files because that kind of huge change breaks git blame (because you appear author of those lines while you are not, so if something breaks somebody may think you are the person to ask why something is not working)
Added jslint config file and jslint support to Travis CI build.
|
I can't claim this task again, because deadline is over. But i don't care about code-in, so you can just ignore it |
|
@frogsandsocks do you want to finish this PR or should I close it? |
|
@frogsandsocks sorry, didn't check that you did address my comments, thanks 👍 |
|
@nemesisdesign |
3 participants
Added jslint support to Travis CI build.