Add a prettier config #46

ismail-syed · 2017-10-12T21:34:05Z

Created and exposed a prettier config that can be consumed for JS projects via plugin:shopify/prettier.
Formatted this repo with the new config

This PR also adds the following dependencies:

prettier: Core prettier formatter.
eslint-plugin-prettier: Exposes prettier as an eslint rule
eslint-config-prettier: A maintained config that has conflicting rules disabled.

Ran this against core, here's a diff to get an idea of potential changes. Let me know if you see any concerns.

ismail-syed · 2017-10-12T21:36:33Z

lib/rules/restrict-full-import.js


    function isRestrictedModule(mod) {
      return restricted.indexOf(mod) >= 0;
    }

+    /* eslint-disable prettier/prettier*/


eslint --fix with the prettier/prettier config breaks this. It outputs invalid code. Create a reproducible issue in issue in eslint-plugin-prettier.

I can confirm that this is a bug.
See prettier/eslint-config-prettier#31 & prettier/eslint-plugin-prettier#65

Instead of disabling prettier here, you can run eslint --fix, manually correct the syntax error, and the eslint --fix again.

Thanks for the tip.
I'd prefer leaving as is. I've commented a link to the issue for context.

ismail-syed · 2017-10-12T21:36:41Z

lib/rules/sinon-prefer-meaningful-assertions.js

    }

+    /* eslint-disable prettier/prettier*/


Same here, eslint --fix with the prettier/prettier config breaks this. It outputs invalid code. Create a reproducible issue in issue in eslint-plugin-prettier.

See prettier/eslint-config-prettier#31

Space before the last *

ismail-syed · 2017-10-12T21:40:51Z

tests/.eslintrc.js

@@ -2,5 +2,6 @@ module.exports = {
  extends: 'plugin:shopify/esnext',
  rules: {
    'import/no-extraneous-dependencies': 'off',
+    'prettier/prettier': 'off',


Alternatively, we could extend the plugin:shopify/prettier config and run prettier against the /tests code.

I like the alternative better

ismail-syed · 2017-10-12T21:41:44Z

package.json

@@ -18,6 +18,8 @@
    "rules-status": "eslint-index lib/config/all.js  --format table",
    "rules-omitted": "eslint-index lib/config/all.js  --status omitted",
    "lint": "eslint . --max-warnings 0",
+    "format": "prettier --write --single-quote --no-bracket-spacing --trailing-comma es5 './**/*.js'",
+    "eslint-check": "eslint --print-config .eslintrc.js | eslint-config-prettier-check",


As we update prettier and/or eslint hopefully, this can continue to list conflicting rules.

ismail-syed · 2017-10-18T19:27:32Z

package.json

@@ -18,6 +18,7 @@
    "rules-status": "eslint-index lib/config/all.js  --format table",
    "rules-omitted": "eslint-index lib/config/all.js  --status omitted",
    "lint": "eslint . --max-warnings 0",
+    "prettier-check": "eslint --print-config .eslintrc.js | eslint-config-prettier-check",


.eslintrc.js should point to ~~esnext~~ prettier config instead.

lemonmade · 2017-10-18T21:03:31Z

lib/config/jquery.js

@@ -3,9 +3,7 @@ module.exports = {
    jquery: true,
  },

-  plugins: [
-    'shopify',
-  ],


Are these changes the result of running prettier?

Yup, it's within the 80 column size .

lemonmade · 2017-10-18T21:04:19Z

lib/config/prettier.js

+    ],
+
+    // rules to disable to prefer prettier
+    'shopify/binary-assignment-parens': 'off',


Can you explain this more

shopify/binary-assignment-parens expects something like this:

const always = (config === 'always');

This will cause prettier to complain:

$ eslint . --max-warnings 0 /Users/ismailsyed/src/github.com/Shopify/eslint-plugin-shopify/lib/rules/class-property-semi.js 14:20 error Replace `(config·===·'always')` with `config·===·'always'` prettier/prettier

Unfortunately, we'll have to turn this rule off in favor of the 'prettier' experience.
No pun intended, lol.

lemonmade · 2017-10-18T21:04:46Z

lib/config/prettier.js

+      'error',
+      {
+        singleQuote: true,
+        trailingComma: 'es5',


Hmm, I think we need to change this for the ESNext config — any way of making this dynamic based on other configs passed in?

The prettier config will disable the rule(s) regardless of the config passed in.

Extending this prettier config forces you to opt-in to the opinionated prettier experience. If a consumer wants, they can update the prettier/prettier rules in their config.

'prettier/prettier': [ 'error', { singleQuote: true, trailingComma: '<none|es5|all>', bracketSpacing: false, jsxBracketSameLine: false, }, ],

lemonmade · 2017-10-18T21:06:08Z

lib/rules/sinon-prefer-meaningful-assertions.js

    }

+    /* eslint-disable prettier/prettier*/


Space before the last *

lemonmade · 2017-10-18T21:08:27Z

tests/.eslintrc.js

@@ -2,5 +2,6 @@ module.exports = {
  extends: 'plugin:shopify/esnext',
  rules: {
    'import/no-extraneous-dependencies': 'off',
+    'prettier/prettier': 'off',


I like the alternative better

lemonmade · 2017-10-18T21:09:08Z

lib/config/flow.js

-    'flowtype',
-    'shopify',
-  ],
+  plugins: ['flowtype', 'shopify'],


There's also a config for prettier/flowtype (in addition to one for React) — do we want those?

Yup. That work can be in a future PR. We'll have to manually find out which rules from those configs we'll need to turn off.

ismail-syed · 2017-10-19T20:16:27Z

Comments addressed, ready for another 👀

GoodForOneFare

It's a clever idea to show the effect of Prettier in this PR, but it's making it difficult to sift out what has really changed.

From my first sift through, two things that I'm not comfortable with:

Arrays being translated to single lines based on line width
- This seems at odds with our "trailing comma makes it easier to track the originator of a change" philosophy
[{ being rewritten to have the opening brace on a new line

The second thing I'm sure I'll get over. The first seems like it should be up to the developer.

ismail-syed · 2017-10-23T14:43:31Z

I kept all the automated --fix with the prettier changes in this one commit, 5c2baf3. I should have mentioned that in the PR description, sorry.
It might be helpful to view it in the individual commit instead.

ismail-syed · 2017-10-23T17:33:03Z

package.json

@@ -49,6 +51,7 @@
    "isparta": "^4.0.0",
    "istanbul": "^0.4.5",
    "mocha": "^3.4.2",
+    "prettier": "^1.7.2",


Move to dependencies

So consumers don't have to install prettier.

I just worry about it needing to be aligned across repos, technically they would need to install it now in order to run the command alongside the linter

Spoke offline with Chris. We're going to add prettier as an optional dependency since prettier is only required when extending the prettier config.

lemonmade

This feels fine to me, I'm OK with the occasional non-ideal formatting.

ismail-syed commented Oct 12, 2017

View reviewed changes

ismail-syed force-pushed the prettier-v1.7.2 branch from 69e3495 to 9308d0d Compare October 12, 2017 21:38

ismail-syed commented Oct 12, 2017

View reviewed changes

ismail-syed force-pushed the prettier-v1.7.2 branch from 9308d0d to 5c2baf3 Compare October 12, 2017 21:43

ismail-syed changed the title ~~[WIP] Add a prettier config~~ Add a prettier config Oct 17, 2017

ismail-syed requested review from GoodForOneFare and lemonmade October 18, 2017 15:57

ismail-syed commented Oct 18, 2017

View reviewed changes

lemonmade reviewed Oct 18, 2017

View reviewed changes

GoodForOneFare reviewed Oct 22, 2017

View reviewed changes

ismail-syed force-pushed the prettier-v1.7.2 branch from 5b14b27 to 1ed02b6 Compare October 23, 2017 16:01

ismail-syed commented Oct 23, 2017

View reviewed changes

ismail-syed added 13 commits October 24, 2017 09:58

Setup prettier config

c7aeea5

eslint --fix with eslint-plugin-prettier on this breaks

0a534d7

Disable prettier on test source

fcbe812

Run eslint --fix with prettier config

33737c4

Remove format script"

924c210

Rename script to prettier-check

d7775d7

Remove uncessary rules

000a1bb

Add a small note about prettier

9982164

Context for disabling

c97d215

PR fixes

9a1f7b2

Run prettier against /tests

9b46224

Point prettier-check to correct config

da9cf95

v17.2.0-alpha.1

4530473

ismail-syed force-pushed the prettier-v1.7.2 branch from e5568ee to d0b3aa5 Compare October 24, 2017 14:08

ismail-syed added 2 commits October 24, 2017 10:11

Add prettier as optional dep

032cf31

Reformat after rebase

eca636a

ismail-syed force-pushed the prettier-v1.7.2 branch from d0b3aa5 to eca636a Compare October 24, 2017 14:11

Fix lint error

8f56a38

lemonmade approved these changes Oct 25, 2017

View reviewed changes

GoodForOneFare approved these changes Oct 25, 2017

View reviewed changes

Change version back to master version

d5c4681

ismail-syed merged commit 45504b6 into master Oct 25, 2017

kaelig deleted the prettier-v1.7.2 branch January 12, 2018 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a prettier config #46

Add a prettier config #46

ismail-syed commented Oct 12, 2017 •

edited

ismail-syed Oct 12, 2017

ismail-syed Oct 16, 2017 •

edited

lydell Oct 16, 2017

ismail-syed Oct 18, 2017 •

edited

ismail-syed Oct 12, 2017

ismail-syed Oct 16, 2017

lemonmade Oct 18, 2017

ismail-syed Oct 12, 2017

lemonmade Oct 18, 2017

ismail-syed Oct 12, 2017

ismail-syed Oct 18, 2017 •

edited

lemonmade Oct 18, 2017

ismail-syed Oct 19, 2017

lemonmade Oct 18, 2017

ismail-syed Oct 19, 2017 •

edited

lemonmade Oct 18, 2017

ismail-syed Oct 19, 2017 •

edited

lemonmade Oct 18, 2017

lemonmade Oct 18, 2017

lemonmade Oct 18, 2017

ismail-syed Oct 19, 2017

ismail-syed commented Oct 19, 2017

GoodForOneFare left a comment

ismail-syed commented Oct 23, 2017 •

edited

ismail-syed Oct 23, 2017

lemonmade Oct 23, 2017

ismail-syed Oct 23, 2017

lemonmade Oct 23, 2017

ismail-syed Oct 23, 2017

lemonmade left a comment

Add a prettier config #46

Add a prettier config #46

Conversation

ismail-syed commented Oct 12, 2017 • edited

Choose a reason for hiding this comment

ismail-syed Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-syed Oct 18, 2017 • edited

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

ismail-syed Oct 18, 2017 • edited

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

ismail-syed Oct 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-syed Oct 19, 2017 • edited

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

ismail-syed commented Oct 19, 2017

GoodForOneFare left a comment

Choose a reason for hiding this comment

ismail-syed commented Oct 23, 2017 • edited

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

lemonmade left a comment

Choose a reason for hiding this comment

ismail-syed commented Oct 12, 2017 •

edited

ismail-syed Oct 16, 2017 •

edited

ismail-syed Oct 18, 2017 •

edited

ismail-syed Oct 18, 2017 •

edited

ismail-syed Oct 19, 2017 •

edited

ismail-syed Oct 19, 2017 •

edited

ismail-syed commented Oct 23, 2017 •

edited