caseycesari
commented
5 years ago @mmcfarland @kellyi Tagging both of you since this is our first use of Prettier and lint-staged.
Closed caseycesari closed 5 years ago
caseycesari
commented
5 years ago @mmcfarland @kellyi Tagging both of you since this is our first use of Prettier and lint-staged.
caseycesari
commented
5 years ago One thing I forgot to mention above is that you can configure Prettier to run each time a file is saved. I thought that was too intrusive so didn't introduce that here. It also depends on which editor people are using. More details here: https://prettier.io/docs/en/editors.html
kellyi
commented
5 years ago Wrote some code for the app component. Here's the diff pre-Prettier:
diff --git a/src/app/src/App.js b/src/app/src/App.js
index 4acce3d..2012f9f 100644
--- a/src/app/src/App.js
+++ b/src/app/src/App.js
@@ -1,21 +1,73 @@
import React, { Component } from 'react';
import { number } from 'prop-types';
import { connect } from 'react-redux';
+import flow from 'lodash/flow';
+import partial from 'lodash/partial';
import logo from './logo.svg';
import './App.scss';
import { updateCount } from './actions';
+const addFourNumbers = (a, b, c, d) => a + b + c + d;
+
+const addThreeNumbers = partial(addFourNumbers, [0]);
+
+const addTwoNumbers = partial(
+ addFourNumbers,
+ [
+ 0,
+ 0,
+ ],
+);
+
+const identity = partial(
+ addFourNumbers,
+ Array
+ .from("abc")
+ .fill(0),
+);
+
+
+const doubleThenTripleThenQuadrupleNumber = flow(
+ identity,
+ z => addTwoNumbers(z, z),
+ z => addThreeNumbers(
+ z,
+ z, z,
+ ),
+ z => addFourNumbers(
+ z,
+ z,
+ z,
+ z,
+ ),
+ z =>
+ identity(z),
+);
+
class App extends Component {
constructor(props) {
super(props);
this.inc = this.inc.bind(this);
+
+ this.processCounter = this
+ .processCounter
+ .bind(this);
}
inc() {
updateCount();
}
+ processCounter() {
+ return this.props.n
+ ? doubleThenTripleThenQuadrupleNumber(
+ this.props.n,
+ )
+ : null;
+ }
+
render() {
return (
<div className="App">
@@ -35,7 +87,21 @@ class App extends Component {
<p>
<button onClick={() => this.inc()}>Click Me</button>
</p>
- <p>{this.props.n}</p>
+ {
+ this.props.n
+ && (
+ <ul>
+ {
+ [...new Array(this.props.n).keys()]
+ .map(n => (
+ <li key={n}>
+ {this.processCounter()}
+ </li>
+ ))
+ }
+ </ul>)
+ }
</header>
</div>
);Hm, I committed the code above but did not see prettier run. I think I had the server stopped though. Trying again with the container running.
kellyi
commented
5 years ago Tried it again with the server running and didn't see it work again.
I ended up running it manually in the container, although I don't know whether it had the same options as were set in the pre-commit hook.
const addFourNumbers = (a, b, c, d) => a + b + c + d;
const addThreeNumbers = partial(addFourNumbers, [0]);
const addTwoNumbers = partial(addFourNumbers, [0, 0]);
const identity = partial(addFourNumbers, Array.from("abc").fill(0));
const doubleThenTripleThenQuadrupleNumber = flow(
identity,
z => addTwoNumbers(z, z),
z => addThreeNumbers(z, z, z),
z => addFourNumbers(z, z, z, z),
z => identity(z)
);
class App extends Component {
constructor(props) {
super(props);
this.inc = this.inc.bind(this);
this.processCounter = this.processCounter.bind(this);
}
inc() {
updateCount();
}
processCounter() {
return this.props.n
? doubleThenTripleThenQuadrupleNumber(this.props.n)
: null;
}
render() {
return (
<div className="App">
<header className="App-header">
<img src={logo} className="App-logo" alt="logo" />
<p>
Edit <code>src/App.js</code> and save to reload.
</p>
<a
className="App-link"
href="https://reactjs.org"
target="_blank"
rel="noopener noreferrer"
>
Learn React
</a>
<p>
<button onClick={() => this.inc()}>Click Me</button>
</p>
{this.props.n && (
<ul>
{[...new Array(this.props.n).keys()].map(n => (
<li key={n}>{this.processCounter()}</li>
))}
</ul>
)}
</header>
</div>
);
}
}I believe all of the original code was valid syntax under the ESLint config. We could probably do away with some of the verticality, but one thing I don't super love in particular about the output is how it smushes the JSX syntax all together, which feels like it's much more dense and harder to understand.
That said I'm not sure I have the same config options as the git hook uses. I ran it in the container via node_modules/.bin/prettier --single-quote --fix src/* which seemed roughly similar.
caseycesari
commented
5 years ago That looks like almost the same config, except that it is using 2 spaces instead of 4. Not sure why that is. I'm adding a Prettier config file to this PR. Also creating a little demo off the example code you added. It's a great example.
caseycesari
commented
5 years ago Since the available Prettier configuration is pretty minimal, I went ahead and added a configuration file that matches our existing eslint config when possible. I reran the files through Prettier to update them. Adding the config file should also help keep things consistent when running Prettier via the CLI.
kellyi
commented
5 years ago Since the available Prettier configuration is pretty minimal, I went ahead and added a configuration file that matches our existing eslint config when possible.
Sounds good. Aside from the trailing comma change, I'm content with trying out this setup's other formatting choices.
I think the only thing that remains to be fixed is to figure out how to ensure the thing will always run on git commits -- I just commited the following patch on this branch and it did not run prettier on commit:
From 12af7fefb14740b323affc082fe84daa98165c47 Mon Sep 17 00:00:00 2001
From: Kelly Innes <kinnes@azavea.com>
Date: Mon, 17 Dec 2018 10:10:22 -0500
Subject: [PATCH] Create list
---
src/app/src/App.js | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/app/src/App.js b/src/app/src/App.js
index bb6b378..19603f7 100644
--- a/src/app/src/App.js
+++ b/src/app/src/App.js
@@ -36,6 +36,16 @@ class App extends Component {
<button onClick={() => this.inc()}>Click Me</button>
</p>
<p>{this.props.n}</p>
+ <ul>
+ {
+ [...new Array(10).keys()]
+ .map(n => (
+ <li key={n}>
+ {n}
+ </li>
+ ))
+ }
+ </ul>
</header>
</div>
);
--
2.20.0
mmcfarland
commented
5 years ago I also am unable to get it to execute on git commit. Did you manually setup a hook? I don't see any Prettier hooks in my .git directory.
caseycesari
commented
5 years ago @mmcfarland @kellyi Instructions have been updated. Ready for another look. Also, here's a demo of the vim plugin:
kellyi
commented
5 years ago Tried this out using the new testing instructions -- npm install on host, then setup as usual. It still didn't seem to run on making the commit:
Here's some new code, pre commit:
diff --git a/src/app/src/App.js b/src/app/src/App.js
index bb6b378..7d6a3a8 100644
--- a/src/app/src/App.js
+++ b/src/app/src/App.js
@@ -1,11 +1,53 @@
import React, { Component } from 'react';
import { number } from 'prop-types';
import { connect } from 'react-redux';
+import flow from 'lodash/flow';
+import partial from 'lodash/partial';
import logo from './logo.svg';
import './App.scss';
import { updateCount } from './actions';
+const addFourNumbers = (a, b, c, d) => a + b + c + d;
+
+const addThreeNumbers = partial(
+ addFourNumbers,
+ [0],
+);
+
+const addTwoNumbers = partial(addFourNumbers, [
+ 0,
+ 0,
+]);
+
+const identity = partial(
+ addFourNumbers,
+ [
+ 0, 0,
+ 0,
+ ],
+);
+
+const repeatArgumentNTimes = (a, n) =>
+ [...new Array(n)]
+ .fill(a);
+
+const doubleThenTripleThenQuadrupleNumber = flow(identity,
+ z => addTwoNumbers(z,
+ z,
+ ),
+ z =>
+ identity(z),
+ z => addThreeNumbers(
+ ...repeatArgumentNTimes(z, 3),
+ ),
+ z => identity(...repeatArgumentNTimes(z,
+ 1)),
+ z => addFourNumbers(
+ ...repeatArgumentNTimes(z, 4)),
+ identity,
+ );
+
class App extends Component {
constructor(props) {
super(props);
@@ -35,7 +77,11 @@ class App extends Component {
<p>
<button onClick={() => this.inc()}>Click Me</button>
</p>
- <p>{this.props.n}</p>
+ <p>
+ {
+ doubleTheTripleThenQuadrupleNumber(this.props.n)
+ }
+ </p>
</header>
</div>
);And here's the commit itself:
commit 19a254910a52ac0bb8cb010d2df136a7241ea51a
Author: Kelly Innes <kinnes@azavea.com>
Date: Wed Dec 19 10:18:08 2018 -0500
Testing commit
diff --git a/src/app/src/App.js b/src/app/src/App.js
index bb6b378..7d6a3a8 100644
--- a/src/app/src/App.js
+++ b/src/app/src/App.js
@@ -1,11 +1,53 @@
import React, { Component } from 'react';
import { number } from 'prop-types';
import { connect } from 'react-redux';
+import flow from 'lodash/flow';
+import partial from 'lodash/partial';
import logo from './logo.svg';
import './App.scss';
import { updateCount } from './actions';
+const addFourNumbers = (a, b, c, d) => a + b + c + d;
+
+const addThreeNumbers = partial(
+ addFourNumbers,
+ [0],
+);
+
+const addTwoNumbers = partial(addFourNumbers, [
+ 0,
+ 0,
+]);
+
+const identity = partial(
+ addFourNumbers,
+ [
+ 0, 0,
+ 0,
+ ],
+);
+
+const repeatArgumentNTimes = (a, n) =>
+ [...new Array(n)]
+ .fill(a);
+
+const doubleThenTripleThenQuadrupleNumber = flow(identity,
+ z => addTwoNumbers(z,
+ z,
+ ),
+ z =>
+ identity(z),
+ z => addThreeNumbers(
+ ...repeatArgumentNTimes(z, 3),
+ ),
+ z => identity(...repeatArgumentNTimes(z,
+ 1)),
+ z => addFourNumbers(
+ ...repeatArgumentNTimes(z, 4)),
+ identity,
+ );
+
class App extends Component {
constructor(props) {
super(props);
@@ -35,7 +77,11 @@ class App extends Component {
<p>
<button onClick={() => this.inc()}>Click Me</button>
</p>
- <p>{this.props.n}</p>
+ <p>
+ {
+ doubleTheTripleThenQuadrupleNumber(this.props.n)
+ }
+ </p>
</header>
</div>
);For reference, I was able to run prettier manually in spacemacs and got this output:
diff --git a/src/app/src/App.js b/src/app/src/App.js
index 7d6a3a8..3ac1077 100644
--- a/src/app/src/App.js
+++ b/src/app/src/App.js
@@ -10,43 +10,23 @@ import { updateCount } from './actions';
const addFourNumbers = (a, b, c, d) => a + b + c + d;
-const addThreeNumbers = partial(
- addFourNumbers,
- [0],
-);
+const addThreeNumbers = partial(addFourNumbers, [0]);
-const addTwoNumbers = partial(addFourNumbers, [
- 0,
- 0,
-]);
+const addTwoNumbers = partial(addFourNumbers, [0, 0]);
-const identity = partial(
- addFourNumbers,
- [
- 0, 0,
- 0,
- ],
-);
+const identity = partial(addFourNumbers, [0, 0, 0]);
-const repeatArgumentNTimes = (a, n) =>
- [...new Array(n)]
- .fill(a);
+const repeatArgumentNTimes = (a, n) => [...new Array(n)].fill(a);
-const doubleThenTripleThenQuadrupleNumber = flow(identity,
- z => addTwoNumbers(z,
- z,
- ),
- z =>
- identity(z),
- z => addThreeNumbers(
- ...repeatArgumentNTimes(z, 3),
- ),
- z => identity(...repeatArgumentNTimes(z,
- 1)),
- z => addFourNumbers(
- ...repeatArgumentNTimes(z, 4)),
- identity,
- );
+const doubleThenTripleThenQuadrupleNumber = flow(
+ identity,
+ z => addTwoNumbers(z, z),
+ z => identity(z),
+ z => addThreeNumbers(...repeatArgumentNTimes(z, 3)),
+ z => identity(...repeatArgumentNTimes(z, 1)),
+ z => addFourNumbers(...repeatArgumentNTimes(z, 4)),
+ identity
+);
class App extends Component {
constructor(props) {
@@ -77,11 +57,7 @@ class App extends Component {
<p>
<button onClick={() => this.inc()}>Click Me</button>
</p>
- <p>
- {
- doubleTheTripleThenQuadrupleNumber(this.props.n)
- }
- </p>
+ <p>{doubleTheTripleThenQuadrupleNumber(this.props.n)}</p>
</header>
</div>
);@kellyi Glad you got it running in your editor. I'm at a loss as to why the precommit hook isn't working, though I'm not sure it's prudent to spend anymore time debugging it. After trying out the vim plugin, I think I'm going to go with that personally, instead of the precommit hook.
kellyi
commented
5 years ago After trying out the vim plugin, I think I'm going to go with that personally, instead of the precommit hook.
Sounds fine to me, let's try it out on this project. In other projects we can use our standard app template setup instead of CRA and thereby be able to add the prettier loader to the webpack config.
mmcfarland
commented
5 years ago I installed vim-prettier and it works nicely in a manual mode, though I'm not sure if it's picking up any local project config rules. I'm also happy to abandon prettier for this project instead of do an unusual workaround and try again on a non-CRA project.
caseycesari
commented
5 years ago vim-prettier is respecting the project configuration for me. I agree that this would be better if it was integrated in webpack, but will leave it for now to get more experience with it.
Thanks for the thorough reviews everyone!
Overview
Add Prettier to the project as well as a script to run ESLint from the command line.
While we have historically used ESLint for both code and style issues, Prettier is used here to fix style issues. There is potential for conflict between the tools, and libraries to get around them. However, we are currently using the ESLint rules that come with CRA. The authors of CRA decided to only have ESLint flag code issues. They recommend Prettier for code style. If we end up running into conflicts, we can add some further configuration.
Connects #8
Demo
Testing Instructions
src/appfolder, runyarn installornpm installto setup the prettier commit hook../scripts/update.