Skip to content

Lint with eslint #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jan 11, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
node_modules
dist
build

test/jasmine/assets/jquery-1.8.3.min.js
src/plots/polar/micropolar.js
src/plots/geo/projections.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add node_modules in here too, no?

I see that projections is a forked file from somewhere else, but why ignore micropolar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add node_modules in here too, no?

Good point. I think eslint ignores it by default. But might as well throw it there for completeness

I see that projections is a forked file from somewhere else, but why ignore micropolar?

Because, that file is brutal and is on its deathbed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for honesty 🍻

40 changes: 40 additions & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"root": true,
"extends": [
"eslint:recommended",
],
"env": {
"commonjs": true
},
"rules": {
"no-trailing-spaces": [2],
"no-multiple-empty-lines": [2, {"max": 3, "maxEOF": 1}],
"linebreak-style": [2, "unix"],
"indent": [2, 4, {"SwitchCase": 1}],
"max-len": [0, 80],
"brace-style": [0, "stroustrup", {"allowSingleLine": true}],
"curly": [0, "multi"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another slightly controversial rule: http://eslint.org/docs/rules/curly

"camelcase": [0, {"properties": "never"}],
"comma-spacing": [0, {"before": false, "after": true}],
"comma-style": [2, "last"],
"semi": [2],
"semi-spacing": [2, {"before": false, "after": true}],
"key-spacing": [0, {"beforeColon": false, "afterColon": true}],
"no-spaced-func": [2],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"space-in-parens": [2, "never"],
"space-before-function-paren": [2, "never"],
"no-multi-spaces": [2],
"space-infix-ops": [0, {"int32Hint": false}],
"quotes": [2, "single"],
"dot-notation": [2, {"allowKeywords": false}],
"eqeqeq": [2],
"new-cap": [0],
"no-redeclare": [2, {"builtinGlobals": true}],
"no-shadow": [0, {"builtinGlobals": true}],
"block-scoped-var": [2],
"no-unused-vars": [2],
"no-use-before-define": [2, "nofunc"],
"no-loop-func": [2],
"no-console": [0]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the rules with [0, /* ... */] are ignored by npm run lint. [1, /* .. */] spit out warnings whereas [2, /* ... */] throws errors.

The rules I added with [2] will be enforced starting now.

The rules I added with [0] try to replicate the style of greater part of the code base.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

},
}
72 changes: 0 additions & 72 deletions .jshintrc

This file was deleted.

9 changes: 5 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,9 @@ To view the results of a run on CircleCI, download the `build/test_images/` and

### Coding style

- 4-space indentation
- semi-colons are required
- trailing commas

Check if ok, with `npm run lint`

- See [eslintrc](https://github.com/plotly/plotly.js/blob/master/.eslintrc) and
the eslint [list of rules](http://eslint.org/docs/rules/) for more details.
- Rules listed in the eslintrc file with the ignore flag `0` are the recommended
rules for new code added.
7 changes: 7 additions & 0 deletions devtools/.eslintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"extends": "../.eslintrc",
"env": {
"node": true,
"browser": true
}
}
4 changes: 2 additions & 2 deletions devtools/image_viewer/viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function createButton(imageName) {
button.style.height = '40px';
button.innerHTML = imageName;

button.addEventListener('click', function () {
button.addEventListener('click', function() {
var imgBaseline = createImg(dirBaseline, imageName),
imgTest = createImg(dirTest, imageName),
imgDiff = createImg(dirDiff, 'diff-' + imageName);
Expand All @@ -50,7 +50,7 @@ function createButton(imageName) {

$mock.innerHTML = '';
$mock.appendChild(createJSONview(mock));
});
});
});

return button;
Expand Down
54 changes: 28 additions & 26 deletions devtools/test_dashboard/buttons.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* global Plotly:false Tabs:false */

var plotlist = document.getElementById('plot-list');
var anchor = document.getElementById('embedded-graph');
var image = document.getElementById('embedded-image');
Expand All @@ -11,7 +13,7 @@ anchor.style.width = '1000px';

function plotButtons(plots, figDir) {

Object.keys(plots).forEach( function (plotname) {
Object.keys(plots).forEach(function(plotname) {

var button = document.createElement('button');

Expand All @@ -23,7 +25,7 @@ function plotButtons(plots, figDir) {

plotlist.appendChild(button);

button.addEventListener('click', function () {
button.addEventListener('click', function() {

var myImage = new Image();
myImage.src = figDir + plotname + '.png';
Expand Down Expand Up @@ -55,7 +57,7 @@ function plotButtons(plots, figDir) {

plotlist.appendChild(snapshot);

snapshot.addEventListener('click', function () {
snapshot.addEventListener('click', function() {

/*
* Grab the currently loaded plot and make an image - replacing the plot.
Expand All @@ -67,7 +69,7 @@ function plotButtons(plots, figDir) {

if (!layout || !data) return;

Plotly.Plots.getSubplotIds(gd._fullLayout, 'gl3d').forEach( function (key) {
Plotly.Plots.getSubplotIds(gd._fullLayout, 'gl3d').forEach(function(key) {
var scene = gd._fullLayout[key]._scene;
scene.destroy();
});
Expand All @@ -80,23 +82,23 @@ function plotButtons(plots, figDir) {
/*
* Replot with staticPlot
*/
Plotly.plot(gd, data, layout, {staticPlot: true, plotGlPixelRatio: 2}).then( function () {
Plotly.Plots.getSubplotIds(gd._fullLayout, 'gl3d').forEach( function (key) {
var scene = gd._fullLayout[key]._scene;
var dataURL = scene.toImage();

var myImage = new Image();
myImage.src = dataURL;

myImage.onload = function () {
myImage.height = scene.container.clientHeight;
myImage.width = scene.container.clientWidth;
};

image.innerHTML = '';
image.appendChild(myImage);
});
})
Plotly.plot(gd, data, layout, {staticPlot: true, plotGlPixelRatio: 2}).then(function() {
Plotly.Plots.getSubplotIds(gd._fullLayout, 'gl3d').forEach(function(key) {
var scene = gd._fullLayout[key]._scene;
var dataURL = scene.toImage();

var myImage = new Image();
myImage.src = dataURL;

myImage.onload = function() {
myImage.height = scene.container.clientHeight;
myImage.width = scene.container.clientWidth;
};

image.innerHTML = '';
image.appendChild(myImage);
});
});
});

var pummelButton = document.createElement('button');
Expand All @@ -112,17 +114,17 @@ function plotButtons(plots, figDir) {
var mock = require('@mocks/gl3d_marker-color.json');
var statusDiv = document.getElementById('status-info');

pummelButton.addEventListener('click', function () {
setInterval(function () {
pummelButton.addEventListener('click', function() {
setInterval(function() {
var plotDiv = document.createElement('div');
window.plotDiv = plotDiv;

plotDiv.id = 'div' + i;
document.body.appendChild(plotDiv);

Plotly.plot(plotDiv, mock.data, mock.layout, {staticPlot: true}).then(function () {
Plotly.plot(plotDiv, mock.data, mock.layout, {staticPlot: true}).then(function() {

Plotly.Plots.getSubplotIds(plotDiv._fullLayout, 'gl3d').forEach( function (key) {
Plotly.Plots.getSubplotIds(plotDiv._fullLayout, 'gl3d').forEach(function(key) {
var scene = plotDiv._fullLayout[key]._scene;
scene.destroy();
i ++;
Expand All @@ -144,7 +146,7 @@ function plotButtons(plots, figDir) {
scrapeButton.style.background = 'blue';
plotlist.appendChild(scrapeButton);

scrapeButton.addEventListener('click', function () {
scrapeButton.addEventListener('click', function() {
Plotly.Snapshot.toSVG(Tabs.get());
return;
});
Expand Down
6 changes: 3 additions & 3 deletions devtools/test_dashboard/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ var testFile;
switch(process.argv[2]) {
case 'cartesian':
testFile = 'test_cartesian';
break;
break;
case 'geo':
testFile = 'test_geo';
break;
break;
case 'gl3d':
testFile = 'test_gl3d';
break;
break;
default:
testFile = 'test_gl2d';
}
Expand Down
2 changes: 2 additions & 0 deletions devtools/test_dashboard/test_cartesian.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/*eslint dot-notation: 0*/

var plotButtons = require('./buttons');

var figDir = '../../test/image/baselines/';
Expand Down
2 changes: 2 additions & 0 deletions devtools/test_dashboard/test_geo.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/*eslint dot-notation: 0*/

var plotButtons = require('./buttons');

var figDir = '../../test/image/baselines/geo_';
Expand Down
2 changes: 2 additions & 0 deletions devtools/test_dashboard/test_gl2d.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/*eslint dot-notation: 0*/

var plotButtons = require('./buttons');

var figDir = '../../test/image/baselines/gl2d_';
Expand Down
2 changes: 2 additions & 0 deletions devtools/test_dashboard/test_gl3d.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/*eslint dot-notation: 0*/

var plotButtons = require('./buttons');

var figDir = '../../test/image/baselines/gl3d_';
Expand Down
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"build": "npm run preprocess && npm run bundle && npm run header",
"cibuild": "node tasks/cibundle.js",
"watch": "node tasks/watch_plotly.js",
"lint": "cd src && jshint . || true",
"lint": "eslint . || true",
"test-jasmine": "karma start test/jasmine/karma.conf.js",
"citest-jasmine": "karma start test/jasmine/karma.ciconf.js",
"test-image": "./tasks/test_image.sh",
Expand Down Expand Up @@ -84,7 +84,6 @@
"falafel": "^1.2.0",
"glob": "^6.0.1",
"jasmine-core": "^2.3.4",
"jshint": "^2.8.0",
"karma": "^0.13.15",
"karma-browserify": "^4.4.1",
"karma-chrome-launcher": "^0.2.1",
Expand Down
9 changes: 9 additions & 0 deletions src/.eslintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"extends": "../.eslintrc",
"env": {
"browser": true
},
"rules": {
"strict": [2, "global"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best part about eslint for me:

We can lint the whole project with no config repetition. eslint is smart enough to extend local and root config options.

}
}
2 changes: 2 additions & 0 deletions src/assets/geo_assets.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
* LICENSE file in the root directory of this source tree.
*/

'use strict';

var saneTopojson = require('sane-topojson');


// export the version found in the package.json
exports.version = require('../../package.json').version;

Expand Down
2 changes: 2 additions & 0 deletions src/components/annotations/arrow_paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* LICENSE file in the root directory of this source tree.
*/

'use strict';

/**
* centerx is a center of scaling tuned for maximum scalability of
Expand All @@ -17,6 +18,7 @@
* TODO: option to have the pointed-to point a little in front of the
* end of the line, as people tend to want a bit of a gap there...
*/

module.exports = [
// no arrow
'',
Expand Down
2 changes: 2 additions & 0 deletions src/components/annotations/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
* LICENSE file in the root directory of this source tree.
*/

'use strict';

var Plotly = require('../../plotly');
var ARROWPATHS = require('./arrow_paths');
var fontAttrs = require('../../plots/font_attributes');
var extendFlat = require('../../lib/extend').extendFlat;


module.exports = {
_isLinkedToArray: true,

Expand Down
Loading