Add code formatting and linting. #952

tech4him1 · 2017-12-20T23:25:23Z

Summary

Add ability to automatically format our code to maintain a consistent code style.

Linting and correct formatting is also enforced through the CI.

Closes #363.

Current Blocking PRs

These PRs will need to be merged first. See #363 (comment) for a simplified TODO list.

Test plan

CI should pass!

verythorough · 2017-12-20T23:28:22Z

Deploy preview for cms-demo ready!

Built with commit f8d1ad2

https://deploy-preview-952--cms-demo.netlify.com

verythorough · 2017-12-20T23:29:23Z

Deploy preview for netlify-cms-www ready!

Built with commit f8d1ad2

https://deploy-preview-952--netlify-cms-www.netlify.com

tech4him1 · 2018-01-03T21:17:40Z

Unfortunately, Prettier does not seem to have many options available.

erquhart · 2018-02-28T16:51:57Z

@tech4him1 I pushed updated commits and changed the config a bit - Prettier produces a lot of stacked args lists, and Babel gets rid of them in the output, so we should just use them everywhere.

Let me know what you think.

tech4him1 · 2018-02-28T17:41:54Z

@erquhart What Prettier options did you change (I don't see the diff since it was a force-push)?

Prettier produces a lot of stacked args lists, and Babel gets rid of them in the output, so we should just use them everywhere.

I don't particular like stacked args lists everywhere, but it's not horrible.

erquhart · 2018-03-02T21:55:25Z

That was the only difference, changing trailing commas to "all" instead of "es5".

tech4him1 · 2018-03-02T23:09:25Z

@Benaiah had a few suggestions in #1013:

--trailing-commas all
--print-width <something wider than 80>

Also, integration with ESLint could probably be done in a separate PR.

Integrate prettier with eslint:

Make prettier run through eslint --fix

Use prettier-compatible eslint config

Make eslint check for proper code formatting in test suite.

jeddy3

I've quickly highlighted just a handful of things that popped out at me.

As suggested in #1013 (comment), consider adopting just the ESLint and stylelint recommended configs, for now, to avoid the linting tools and Prettier conflicting.

jeddy3 · 2018-03-03T00:00:46Z

package.json

@@ -17,10 +17,16 @@
    "lint:css": "stylelint 'src/**/*.css'",
    "lint:css:fix": "stylefmt --recursive src/",


I believe using Prettier to pretty-printing your CSS code will make stylefmt redundant. It can be safely removed.

jeddy3 · 2018-03-03T00:02:40Z

package.json

    "deps": "npm-check -s",
    "deps:update": "npm-check -u",
    "prepublishOnly": "npm run build"
  },
+  "prettier": {
+    "trailingComma": "all"
+  },
  "lint-staged": {


As lint-staged is already being used, I believe Prettier can be added here (alongside eslint --fix).

jeddy3 · 2018-03-03T00:07:13Z

src/components/App/App.css

@@ -1,5 +1,5 @@
-@import "./NotFoundPage.css";
-@import "./Header.css";
+@import './NotFoundPage.css';


Prettier seems to use double quotes by default (at least within the playground). It feels odd that it has converted these quotes to single ones. Is there a configuration option not shown in this PR that could be causing this?

Prettier's default for quotes is double, so I'm not sure why they choose to change that for imports.

I was wondering about that - the unwritten rule has been single quotes everywhere except JSX attributes, we should configure for that.

jeddy3 · 2018-03-03T00:14:38Z

src/components/App/Header.css

@@ -3,7 +3,7 @@
 }

 .nc-appHeader-main {
-  @apply(--dropShadowMain);
+  @apply (--dropShadowMain);


Even though the use of parentheses isn't mentioned in the @apply spec, postcss-cssnext seems (at least within the playground) to be able to transpile it, including with the additional space introduced in this change.

Incidentally, I believe the @apply feature has been abandoned. I can create a separate issue to discuss whether it should be refactored out if you like?

I'm aware of @apply stalling out, I'm expecting to just drop it whenever we switch to a CSS-in-JS solution rather than migrating away from it under plain CSS.

tech4him1 · 2018-03-05T17:11:03Z

Updated PR to use single quotes -- we will need to re-run Prettier before merge.

erquhart · 2018-03-19T17:11:43Z

Just holding off on this until we're ready to break every PR. We'll get it in soon.

tech4him1 · 2018-03-31T18:43:26Z

Updated ESLint/StyleLint to use only code (not style) checks.

tech4him1 · 2018-04-04T16:43:18Z

package.json

@@ -14,17 +14,24 @@
    "add-contributor": "all-contributors add",
    "generate-contributors": "all-contributors generate",
    "lint": "npm run lint:js & npm run lint:css",
-    "lint:js": "eslint .",
+    "lint:js": "eslint src",


Do we need to lint everything, or just the source files?

I'd still want to lint other dirs like example and website, and top level files such as the webpack configs. If eslint knows to only hit js files and to ignore the dist, this should work.

tech4him1 · 2018-04-12T01:36:37Z

@jeddy3 Should we be running stylelint --fix along with Prettier and eslint --fix?

jeddy3 · 2018-04-12T13:18:06Z

Should we be running stylelint --fix along with Prettier and eslint --fix

I don't believe any of the rules in the recommended config are autofixable. It's mainly the stylistic rules that can be autofixed in stylelint, which aren't included in the recommended config.

So, I don't think there's any harm in using stylelint --fix, but at it won't (at this time) do anything for you.

erquhart · 2018-05-14T17:26:09Z

We should pull the trigger on this right before the v2 release.

argus777

Review content removed

tech4him1 · 2018-08-04T14:53:30Z

Two blocking PRs left, then we're good to go!

erquhart · 2018-08-07T15:57:06Z

All blockers merged 🎉 🎉

Last call for anyone that wants to opine on this one - I'll review soon @tech4him1.

tech4him1 · 2018-08-07T16:54:36Z

Awesome, I'll work on rebasing!

tech4him1 changed the title ~~WIP: Add automatic code formatting with Prettier~~ Add automatic code formatting with Prettier Jan 2, 2018

tech4him1 changed the title ~~Add automatic code formatting with Prettier~~ WIP: Add automatic code formatting with Prettier Jan 2, 2018

tech4him1 force-pushed the add-prettier branch 2 times, most recently from 9648718 to f5d5380 Compare January 3, 2018 21:15

tech4him1 changed the title ~~WIP: Add automatic code formatting with Prettier~~ Add automatic code formatting with Prettier Jan 3, 2018

tech4him1 added the status: 1-design-review label Jan 3, 2018

tech4him1 force-pushed the add-prettier branch from ea4e562 to 490344d Compare January 3, 2018 21:53

tech4him1 added the status: 2-code-review label Jan 30, 2018

erquhart force-pushed the add-prettier branch from 490344d to c229487 Compare February 28, 2018 16:40

tech4him1 removed the status: 1-design-review label Mar 2, 2018

erquhart mentioned this pull request Mar 2, 2018

Determine testing strategies #1013

Closed

28 tasks

jeddy3 reviewed Mar 3, 2018

View reviewed changes

tech4him1 force-pushed the add-prettier branch from dda15dc to 3772d28 Compare March 30, 2018 20:38

tech4him1 commented Apr 4, 2018

View reviewed changes

tech4him1 mentioned this pull request Apr 12, 2018

Fix miscellaneous code errors. #1251

Merged

tech4him1 force-pushed the add-prettier branch from 36e4b37 to a1b8b69 Compare April 12, 2018 00:10

argus777 suggested changes Jun 30, 2018

View reviewed changes

tech4him1 force-pushed the add-prettier branch from bcfe9ad to 7178db3 Compare August 1, 2018 16:54

tech4him1 force-pushed the add-prettier branch 2 times, most recently from 6fe4e73 to 7d8faa5 Compare August 7, 2018 18:26

tech4him1 removed the status: 2-code-review label Aug 7, 2018

tech4him1 added 13 commits August 7, 2018 13:21

test(lint): add and configure linters

23533cc

test(lint): lint as part of CI

b06ca09

allow console usage throughout code

cf4dac6

don't error on missing prop-types

4fd4d83

allow descending specificity in website CSS

bd07c3f

chore: update linters

31e4798

only output lint hard errors on Travis

a1eea43

chore(lint): fix remaining errors

fc6734f

add linting to Netlify CI

efb1a6b

update travis script

5626a67

fix stylelint breaking with --fix parameter

cdf8872

make lint-fix run quietly

84c8f60

chore: minor fixes for Prettier/ESLint

0e2d2e4

tech4him1 force-pushed the add-prettier branch 2 times, most recently from 8e3cf80 to 0e2d2e4 Compare August 7, 2018 19:51

tech4him1 added 5 commits August 7, 2018 13:59

update Prettier settings

6653b15

ignore built files

321d77a

run Prettier

f7152b7

line width 100

2641ceb

Prettier re-run

f8d1ad2

erquhart approved these changes Aug 7, 2018

View reviewed changes

erquhart merged commit f801b19 into master Aug 7, 2018

erquhart deleted the add-prettier branch August 7, 2018 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add code formatting and linting. #952

Add code formatting and linting. #952

tech4him1 commented Dec 20, 2017 •

edited

Loading

verythorough commented Dec 20, 2017 •

edited

Loading

verythorough commented Dec 20, 2017 •

edited

Loading

tech4him1 commented Jan 3, 2018 •

edited

Loading

erquhart commented Feb 28, 2018

tech4him1 commented Feb 28, 2018 •

edited

Loading

erquhart commented Mar 2, 2018

tech4him1 commented Mar 2, 2018 •

edited

Loading

jeddy3 left a comment

jeddy3 Mar 3, 2018 •

edited

Loading

jeddy3 Mar 3, 2018

tech4him1 Mar 30, 2018

jeddy3 Mar 3, 2018

tech4him1 Mar 3, 2018 •

edited

Loading

erquhart Mar 5, 2018 •

edited

Loading

jeddy3 Mar 3, 2018 •

edited

Loading

erquhart Mar 5, 2018 •

edited

Loading

tech4him1 commented Mar 5, 2018

erquhart commented Mar 19, 2018

tech4him1 commented Mar 31, 2018

tech4him1 Apr 4, 2018

erquhart Apr 11, 2018

tech4him1 commented Apr 12, 2018

jeddy3 commented Apr 12, 2018

erquhart commented May 14, 2018

argus777 left a comment •

edited by erquhart

Loading

tech4him1 commented Aug 4, 2018

erquhart commented Aug 7, 2018

tech4him1 commented Aug 7, 2018

		@@ -17,10 +17,16 @@
		"lint:css": "stylelint 'src/*/.css'",
		"lint:css:fix": "stylefmt --recursive src/",

Add code formatting and linting. #952

Add code formatting and linting. #952

Conversation

tech4him1 commented Dec 20, 2017 • edited Loading

Summary

Current Blocking PRs

Test plan

verythorough commented Dec 20, 2017 • edited Loading

verythorough commented Dec 20, 2017 • edited Loading

tech4him1 commented Jan 3, 2018 • edited Loading

erquhart commented Feb 28, 2018

tech4him1 commented Feb 28, 2018 • edited Loading

erquhart commented Mar 2, 2018

tech4him1 commented Mar 2, 2018 • edited Loading

jeddy3 left a comment

Choose a reason for hiding this comment

jeddy3 Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

jeddy3 Mar 3, 2018

Choose a reason for hiding this comment

tech4him1 Mar 30, 2018

Choose a reason for hiding this comment

jeddy3 Mar 3, 2018

Choose a reason for hiding this comment

tech4him1 Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

erquhart Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

jeddy3 Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

erquhart Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

tech4him1 commented Mar 5, 2018

erquhart commented Mar 19, 2018

tech4him1 commented Mar 31, 2018

tech4him1 Apr 4, 2018

Choose a reason for hiding this comment

erquhart Apr 11, 2018

Choose a reason for hiding this comment

tech4him1 commented Apr 12, 2018

jeddy3 commented Apr 12, 2018

erquhart commented May 14, 2018

argus777 left a comment • edited by erquhart Loading

Choose a reason for hiding this comment

tech4him1 commented Aug 4, 2018

erquhart commented Aug 7, 2018

tech4him1 commented Aug 7, 2018

tech4him1 commented Dec 20, 2017 •

edited

Loading

verythorough commented Dec 20, 2017 •

edited

Loading

verythorough commented Dec 20, 2017 •

edited

Loading

tech4him1 commented Jan 3, 2018 •

edited

Loading

tech4him1 commented Feb 28, 2018 •

edited

Loading

tech4him1 commented Mar 2, 2018 •

edited

Loading

jeddy3 Mar 3, 2018 •

edited

Loading

tech4him1 Mar 3, 2018 •

edited

Loading

erquhart Mar 5, 2018 •

edited

Loading

jeddy3 Mar 3, 2018 •

edited

Loading

erquhart Mar 5, 2018 •

edited

Loading

argus777 left a comment •

edited by erquhart

Loading