[EXPERIMENTAL] Prettier! #1629

rreusser · 2017-04-26T22:15:18Z

Just a quick test with prettier. Adds the following features:

replace lint with prettier-check (to assert PRs satisfy prettier formatting)
replace lint-fix with prettier --write (to apply formatting)
husky + lint-staged to automatically apply prettier formatting to staged files when committing

Except for having to read the code (not to be taken for granted without consideration), this would hopefully make prettier zero-overhead without any effort in transitioning (as opposed to standard which will require a lot of manual formatting for what it's unable to fix.)

Note: current failure seems to be regex-based metadata-stripping browserify transform. Would need a couple tweaks to recognize line breaks.

etpinard · 2017-04-27T00:21:24Z

husky + lint-staged to automatically apply prettier formatting to staged files when committing

Too magical for my taste. But yeah, nice try.

rreusser · 2017-04-27T00:30:29Z

Agreed on the magic, but grep shows at least 260 commit messages containing the word "lint". I forget sooo often.

etpinard · 2017-04-27T00:32:58Z

I forget sooo often.

Nothing stops you from setting this up locally 🍻

rreusser · 2017-04-27T00:35:53Z

Yeah, I tried once but didn't get the exit if details correct so that it mostly just didn't work. (should be real simple though, right?) So I elected to just deal with forgetting sometimes.

n-riesco · 2017-04-27T07:47:39Z

lib/index-basic.js

-    require('./bar'),
-    require('./pie')
-]);
+Plotly.register([require('./bar'), require('./pie')]);


Is is possible to configure prettier to respect vertical formatting?

I see the need to split long lines, but joining short one into a single line doesn't help here.

n-riesco · 2017-04-27T07:48:28Z

lib/index-cartesian.js

-    require('./pie'),
-    require('./contour'),
-    require('./scatterternary')
+  require('./bar'),


Is it possible to configure prettier to use a 4-space indent?

Yes. See: https://github.com/prettier/prettier#api

n-riesco · 2017-04-27T07:49:46Z

lib/index-cartesian.js

+  require('./histogram2dcontour'),
+  require('./pie'),
+  require('./contour'),
+  require('./scatterternary'),


👍 for comma-ending last elements (neater diffs)

n-riesco · 2017-04-27T07:51:47Z

package.json

    "bundle": "node tasks/bundle.js",
    "header": "node tasks/header.js",
    "stats": "node tasks/stats.js",
    "build": "npm run preprocess && npm run bundle && npm run header && npm run stats",
    "cibuild": "npm run preprocess && node tasks/cibundle.js",
    "watch": "node tasks/watch.js",
-    "lint": "eslint --version && eslint .",


is prettier able to detect unused variables and variables used out of scope?

If not, I think it's better to keep using eslint for linting.

Agreed. Asking on the gitter channel just in case since I wasn't able to find a better answer, but I think that alone might be enough for me. Seems some people run it through both, but that seems a bit annoying (though not necessarily slow if you prettier + eslint only staged files).

n-riesco · 2017-04-27T07:53:37Z

src/components/annotations/annotation_defaults.js


-    var borderWidth = coerce('borderwidth');
-    var showArrow = coerce('showarrow');
+  if (!(visible || clickToShow)) return annOut;


😉 Dammit! Now that I got to train myself not to write a space after if!

The golden rule of conventions: Convention <insert convention you use here> is aesthetically superior to <insert proposed alteration here>. 😉

n-riesco · 2017-04-27T07:55:45Z

src/components/annotations/annotation_defaults.js

-    coerce('width');
-    coerce('align');
+  var borderColor = coerce('bordercolor'),
+    borderOpacity = Color.opacity(borderColor);


(aesthetic comment) 4-space indents make this kind of variable declaration look neater.

n-riesco · 2017-04-27T07:59:59Z

src/components/annotations/calc_autorange.js

+          ppadplus: ann._ypadplus,
+          ppadminus: ann._ypadminus,
+        });
+      } else {


n-riesco · 2017-04-27T08:06:18Z

src/components/annotations/convert_coords.js

-    var toLog = (newType === 'log') && (ax.type === 'linear'),
-        fromLog = (newType === 'linear') && (ax.type === 'log');
+  var toLog = newType === 'log' && ax.type === 'linear',
+    fromLog = newType === 'linear' && ax.type === 'log';


is it possible to disable this kind of change?

Using unnecessary parenthesis to improve readability is a good thing.

I think there are some controls, but not sure how granular you can get.

n-riesco · 2017-04-27T08:11:32Z

src/components/annotations/draw_arrow_head.js

+    if (style > 5) rot = 0; // don't rotate square or circle
+    d3
+      .select(el.parentElement)
+      .append('path')


This one is going to be tricky.

I'd be surprised if prettier respects d3 half-indenting conventions.

👍 to get rid of half-indents.
👎 for not respecting vertical formatting

n-riesco · 2017-04-27T08:13:38Z

src/components/annotations/index.js

-    moduleType: 'component',
-    name: 'annotations',
+  moduleType: 'component',
+  name: 'annotations',



Interesting, here prettier does respect vertical formatting and keeps the empty line.

n-riesco · 2017-04-27T08:14:25Z

src/components/color/attributes.js

-    '#7f7f7f',  // middle gray
-    '#bcbd22',  // curry yellow-green
-    '#17becf'   // blue-teal
+  '#1f77b4', // muted blue


no double-space before inline comment?

n-riesco · 2017-04-27T08:14:54Z

src/components/color/index.js

 'use strict';

 var tinycolor = require('tinycolor2');
 var isNumeric = require('fast-isnumeric');

-var color = module.exports = {};
+var color = (module.exports = {});


n-riesco · 2017-04-27T08:16:46Z

src/components/color/index.js

-    }
+  if (!container || typeof container !== 'object') return;
+
+  var keys = Object.keys(container), i, j, key, val;


another example of not respecting vertical formatting.

n-riesco · 2017-04-27T08:22:46Z

Not surprisingly there are good and bad things about prettier's changes.

I'd like to keep eslint for linting though. I find very useful that it reports possible errors (e.g. unused variables and variables scopes).

rreusser · 2017-04-27T14:13:09Z

Thanks for the good feedback @n-riesco. I'm inclined to agree. I think I'll close this for now as going maybe a little too far, though I'll hold out hope that it becomes the future of linting. 😄 The main thing that sends me in cold sweats for standard is the requirement of manually reworking hundreds of variable declarations and similar things that the auto-formatter isn't able to fix automatically.

Prettier!

402c3ea

rreusser mentioned this pull request Apr 26, 2017

Parcoords fonts #1624

Merged

n-riesco reviewed Apr 27, 2017

View reviewed changes

src/components/annotations/calc_autorange.js

ppadplus: ann._ypadplus,

ppadminus: ann._ypadminus,

});

} else {

Copy link

Contributor

n-riesco Apr 27, 2017

Choose a reason for hiding this comment

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

😉

n-riesco reviewed Apr 27, 2017

View reviewed changes

rreusser closed this Apr 27, 2017

alexcjohnson mentioned this pull request Apr 29, 2017

Lint with standardjs #950

Closed

etpinard deleted the prettier branch May 16, 2017 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

[EXPERIMENTAL] Prettier! #1629

[EXPERIMENTAL] Prettier! #1629

rreusser commented Apr 26, 2017 •

edited

Loading

etpinard commented Apr 27, 2017

rreusser commented Apr 27, 2017

etpinard commented Apr 27, 2017

rreusser commented Apr 27, 2017 •

edited

Loading

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017

rreusser Apr 27, 2017

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017 •

edited

Loading

rreusser Apr 27, 2017

n-riesco Apr 27, 2017 •

edited

Loading

rreusser Apr 27, 2017 •

edited

Loading

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017

rreusser Apr 27, 2017

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017

n-riesco Apr 27, 2017

n-riesco commented Apr 27, 2017

rreusser commented Apr 27, 2017 •

edited

Loading

[EXPERIMENTAL] Prettier! #1629

[EXPERIMENTAL] Prettier! #1629

Conversation

rreusser commented Apr 26, 2017 • edited Loading

etpinard commented Apr 27, 2017

rreusser commented Apr 27, 2017

etpinard commented Apr 27, 2017

rreusser commented Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

rreusser Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco commented Apr 27, 2017

rreusser commented Apr 27, 2017 • edited Loading

rreusser commented Apr 26, 2017 •

edited

Loading

rreusser commented Apr 27, 2017 •

edited

Loading

n-riesco Apr 27, 2017 •

edited

Loading

n-riesco Apr 27, 2017 •

edited

Loading

rreusser Apr 27, 2017 •

edited

Loading

rreusser commented Apr 27, 2017 •

edited

Loading