Uh oh!
There was an error while loading. Please reload this page.
Conversation
etpinard
commented
Apr 27, 2017
Too magical for my taste. But yeah, nice try. |
rreusser
commented
Apr 27, 2017
Agreed on the magic, but grep shows at least 260 commit messages containing the word "lint". I forget sooo often. |
etpinard
commented
Apr 27, 2017
Nothing stops you from setting this up locally 🍻 |
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. |
| require('./bar'), | ||
| require('./pie') | ||
| ]); | ||
| Plotly.register([require('./bar'), require('./pie')]); |
There was a problem hiding this comment.
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.
| require('./pie'), | ||
| require('./contour'), | ||
| require('./scatterternary') | ||
| require('./bar'), |
There was a problem hiding this comment.
Is it possible to configure prettier to use a 4-space indent?
There was a problem hiding this comment.
Yes. See: https://github.com/prettier/prettier#api
| require('./histogram2dcontour'), | ||
| require('./pie'), | ||
| require('./contour'), | ||
| require('./scatterternary'), |
There was a problem hiding this comment.
👍 for comma-ending last elements (neater diffs)
| "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 .", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| var borderWidth = coerce('borderwidth'); | ||
| var showArrow = coerce('showarrow'); | ||
| if (!(visible || clickToShow)) return annOut; |
There was a problem hiding this comment.
😉 Dammit! Now that I got to train myself not to write a space after if!
There was a problem hiding this comment.
The golden rule of conventions: Convention <insert convention you use here> is aesthetically superior to <insert proposed alteration here>. 😉
| coerce('width'); | ||
| coerce('align'); | ||
| var borderColor = coerce('bordercolor'), | ||
| borderOpacity = Color.opacity(borderColor); |
There was a problem hiding this comment.
(aesthetic comment) 4-space indents make this kind of variable declaration look neater.
| ppadplus: ann._ypadplus, | ||
| ppadminus: ann._ypadminus, | ||
| }); | ||
| } else { |
| 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'; |
There was a problem hiding this comment.
is it possible to disable this kind of change?
Using unnecessary parenthesis to improve readability is a good thing.
There was a problem hiding this comment.
I think there are some controls, but not sure how granular you can get.
| if (style > 5) rot = 0; // don't rotate square or circle | ||
| d3 | ||
| .select(el.parentElement) | ||
| .append('path') |
There was a problem hiding this comment.
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
| name: 'annotations', | ||
| moduleType: 'component', | ||
| name: 'annotations', | ||
There was a problem hiding this comment.
Interesting, here prettier does respect vertical formatting and keeps the empty line.
| '#7f7f7f', // middle gray | ||
| '#bcbd22', // curry yellow-green | ||
| '#17becf' // blue-teal | ||
| '#1f77b4', // muted blue |
There was a problem hiding this comment.
no double-space before inline comment?
| var isNumeric = require('fast-isnumeric'); | ||
| var color = module.exports = {}; | ||
| var color = (module.exports = {}); |
| } | ||
| if (!container || typeof container !== 'object') return; | ||
| var keys = Object.keys(container), i, j, key, val; |
There was a problem hiding this comment.
another example of not respecting vertical formatting.
n-riesco
commented
Apr 27, 2017
Not surprisingly there are good and bad things about I'd like to keep |
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. |
Just a quick test with prettier. Adds the following features:
lintwithprettier-check(to assert PRs satisfy prettier formatting)lint-fixwithprettier --write(to apply formatting)husky+lint-stagedto automatically apply prettier formatting to staged files when committingExcept 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.