New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[chore] Added prettier formatting to src and test #2676

Merged
merged 1 commit into from Dec 11, 2017

Conversation

5 participants
@adityavohra7
Contributor

adityavohra7 commented Oct 24, 2017

This branch attempts to add prettier to this project via eslint. The motivation behind doing this is mostly to have easily configurable and consistent JS formatting across the entire project. I chose the values for the prettier options used (semi and singleQuote) based on what I thought the codebase was leaning towards. All other options assume their default values. Let me know if I should configure anything differently! I also ran yarn lint --fix.

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Oct 24, 2017

Contributor

Ah checks failed because I forgot to run yarn examples:lint. I'll update the PR shortly!

Contributor

adityavohra7 commented Oct 24, 2017

Ah checks failed because I forgot to run yarn examples:lint. I'll update the PR shortly!

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Oct 24, 2017

Contributor

Also fixing a failing snapshot test in examples/todos-flow.

Contributor

adityavohra7 commented Oct 24, 2017

Also fixing a failing snapshot test in examples/todos-flow.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Oct 24, 2017

Member

Woah, that's a lot of stuff.

Perhaps we can limit it to just the src and test directory for now? This is going to bust a lot of PRs otherwise.

Also, can we extract the prettier config to a .prettierrc.json file. Editor plugins will be able to pick up on the config that way.

Member

timdorr commented Oct 24, 2017

Woah, that's a lot of stuff.

Perhaps we can limit it to just the src and test directory for now? This is going to bust a lot of PRs otherwise.

Also, can we extract the prettier config to a .prettierrc.json file. Editor plugins will be able to pick up on the config that way.

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Oct 24, 2017

Contributor

Heh, totally fair, it's a pretty large diff 😅 I'll try to restrict prettier to only run on src/ and test/ for now. What about build/ (eslint currently runs on this)?

And hmm I've usually found that editors that have eslint integrations/plugins installed can pick up prettier if it runs through eslint. But I guess those who have prettier plugins installed would need a prettier config file? I'll add one anyway. What do you think about adding a .prettierrc.js? JS might let us do logic if we ever need it?

Contributor

adityavohra7 commented Oct 24, 2017

Heh, totally fair, it's a pretty large diff 😅 I'll try to restrict prettier to only run on src/ and test/ for now. What about build/ (eslint currently runs on this)?

And hmm I've usually found that editors that have eslint integrations/plugins installed can pick up prettier if it runs through eslint. But I guess those who have prettier plugins installed would need a prettier config file? I'll add one anyway. What do you think about adding a .prettierrc.js? JS might let us do logic if we ever need it?

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Oct 24, 2017

Member

That'll be going away in the next major, so I wouldn't worry about it.

I'd go with the JSON .prettierrc for now. I think that has broader support, so it's more likely for someone to pick up in their editor.

Member

timdorr commented Oct 24, 2017

That'll be going away in the next major, so I wouldn't worry about it.

I'd go with the JSON .prettierrc for now. I think that has broader support, so it's more likely for someone to pick up in their editor.

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Oct 25, 2017

Contributor

I've updated the PR to run prettier only on src/ and test/. I've also changed how prettier was being run. It's now being running via its own CLI as opposed to via eslint. I did this because I was having some troubles trying to ignore examples/ when running prettier through eslint. Here's what I changed the .eslintrc.js to be:

module.exports = {
  extends: ['react-app', 'prettier'],

  plugins: ['prettier'],

  rules: {
    'prettier/prettier': ['error', require('./.prettierrc.json')]
  },

  overrides: [
    {
      files: 'test/**/*.js',
      env: {
        jest: true,
      },
    },
  ],
}

where .prettierrc.json is:

{
  "semi": false,
  "singleQuote": true
}

and the .prettierignore is:

es
examples
dist
lib

Running prettier directly on examples/ yields nothing (as expected, examples/ is ignored):

➜  redux git:(add_prettier) ✗ ./node_modules/.bin/prettier 'examples/**/*.js'
➜  redux git:(add_prettier) ✗

But running it through eslint results in errors (examples/ dir is not ignored):

➜  redux git:(add_prettier) ✗ yarn examples:lint
yarn run v1.1.0
$ eslint examples

/Users/adityavohra7/Documents/Github/redux/examples/async/src/components/Picker.js
   7:53  error  Delete `⏎···········`                                                             prettier/prettier
   9:29  error  Insert `·(`                                                                       prettier/prettier
  12:18  error  Delete `)`                                                                        prettier/prettier
  13:7   error  Insert `))`                                                                       prettier/prettier
  19:30  error  Replace `⏎····PropTypes.string.isRequired⏎··` with `PropTypes.string.isRequired`  prettier/prettier

/Users/adityavohra7/Documents/Github/redux/examples/async/src/components/Posts.js
  4:17  error  Replace `posts` with `·posts·`                                                                                                                       prettier/prettier
  5:7   error  Replace `⏎····{posts.map((post,·i)·=>⏎······<li·key={i}>{post.title}</li>⏎····)}⏎··` with `{posts.map((post,·i)·=>·<li·key={i}>{post.title}</li>)}`  prettier/prettier

/Users/adityavohra7/Documents/Github/redux/examples/async/src/containers/App.js
  46:16  error  Insert `⏎·········`                                                                                                                                                                     prettier/prettier
  47:1   error  Delete `······`                                                                                                                                                                         
...
...

I think this might be due to how prettier's API is called in eslint-plugin-prettier? Because examples/ isn't ignored by eslint, it passes down the files in there to prettier too?

Do we have any preferences on how we want to run prettier? Is running it directly through the CLI okay? Or would we want to run it through eslint?

Contributor

adityavohra7 commented Oct 25, 2017

I've updated the PR to run prettier only on src/ and test/. I've also changed how prettier was being run. It's now being running via its own CLI as opposed to via eslint. I did this because I was having some troubles trying to ignore examples/ when running prettier through eslint. Here's what I changed the .eslintrc.js to be:

module.exports = {
  extends: ['react-app', 'prettier'],

  plugins: ['prettier'],

  rules: {
    'prettier/prettier': ['error', require('./.prettierrc.json')]
  },

  overrides: [
    {
      files: 'test/**/*.js',
      env: {
        jest: true,
      },
    },
  ],
}

where .prettierrc.json is:

{
  "semi": false,
  "singleQuote": true
}

and the .prettierignore is:

es
examples
dist
lib

Running prettier directly on examples/ yields nothing (as expected, examples/ is ignored):

➜  redux git:(add_prettier) ✗ ./node_modules/.bin/prettier 'examples/**/*.js'
➜  redux git:(add_prettier) ✗

But running it through eslint results in errors (examples/ dir is not ignored):

➜  redux git:(add_prettier) ✗ yarn examples:lint
yarn run v1.1.0
$ eslint examples

/Users/adityavohra7/Documents/Github/redux/examples/async/src/components/Picker.js
   7:53  error  Delete `⏎···········`                                                             prettier/prettier
   9:29  error  Insert `·(`                                                                       prettier/prettier
  12:18  error  Delete `)`                                                                        prettier/prettier
  13:7   error  Insert `))`                                                                       prettier/prettier
  19:30  error  Replace `⏎····PropTypes.string.isRequired⏎··` with `PropTypes.string.isRequired`  prettier/prettier

/Users/adityavohra7/Documents/Github/redux/examples/async/src/components/Posts.js
  4:17  error  Replace `posts` with `·posts·`                                                                                                                       prettier/prettier
  5:7   error  Replace `⏎····{posts.map((post,·i)·=>⏎······<li·key={i}>{post.title}</li>⏎····)}⏎··` with `{posts.map((post,·i)·=>·<li·key={i}>{post.title}</li>)}`  prettier/prettier

/Users/adityavohra7/Documents/Github/redux/examples/async/src/containers/App.js
  46:16  error  Insert `⏎·········`                                                                                                                                                                     prettier/prettier
  47:1   error  Delete `······`                                                                                                                                                                         
...
...

I think this might be due to how prettier's API is called in eslint-plugin-prettier? Because examples/ isn't ignored by eslint, it passes down the files in there to prettier too?

Do we have any preferences on how we want to run prettier? Is running it directly through the CLI okay? Or would we want to run it through eslint?

@adityavohra7 adityavohra7 changed the title from [chore] Added prettier via eslint to [WIP] [chore] Added prettier via eslint Oct 25, 2017

Show outdated Hide outdated package.json
@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Oct 26, 2017

Contributor

@timdorr, @eps1lon, updated! Added the format and format:check run scripts, and added the format:check script to prepare before lint.

Contributor

adityavohra7 commented Oct 26, 2017

@timdorr, @eps1lon, updated! Added the format and format:check run scripts, and added the format:check script to prepare before lint.

@adityavohra7 adityavohra7 changed the title from [WIP] [chore] Added prettier via eslint to [chore] Added prettier via eslint Oct 30, 2017

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Nov 1, 2017

Contributor

Friendly bump! Let me know if there's anything I should change!

Contributor

adityavohra7 commented Nov 1, 2017

Friendly bump! Let me know if there's anything I should change!

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Nov 1, 2017

Member

Just a question: Do you know if prettier --list-differences will return a non-zero exit code (e.g., indicate a failure)? If so, we're all good here. Otherwise, style violating code can still make its way through.

Member

timdorr commented Nov 1, 2017

Just a question: Do you know if prettier --list-differences will return a non-zero exit code (e.g., indicate a failure)? If so, we're all good here. Otherwise, style violating code can still make its way through.

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Nov 1, 2017

Contributor

Yup! Looks like it does. With this diff:

diff --git a/src/bindActionCreators.js b/src/bindActionCreators.js
index b44d6ff..6b02dfd 100644
--- a/src/bindActionCreators.js
+++ b/src/bindActionCreators.js
@@ -27,7 +27,7 @@ function bindActionCreator(actionCreator, dispatch) {
  */
 export default function bindActionCreators(actionCreators, dispatch) {
   if (typeof actionCreators === 'function') {
-    return bindActionCreator(actionCreators, dispatch)
+    return bindActionCreator(actionCreators, dispatch);
   }

   if (typeof actionCreators !== 'object' || actionCreators === null) {

Running the prepare script yields:

➜  redux git:(add_prettier) ✗ yarn prepare
yarn run v1.1.0
$ npm run clean && npm run format:check && npm run lint && npm test && npm run build

> redux@3.7.2 clean /Users/adityavohra7/Documents/Github/redux
> rimraf lib dist es coverage

> redux@3.7.2 format:check /Users/adityavohra7/Documents/Github/redux
> prettier --list-different '{src,test}/**/*.js'

src/bindActionCreators.js
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! redux@3.7.2 format:check: `prettier --list-different '{src,test}/**/*.js'`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the redux@3.7.2 format:check script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/adityavohra7/.npm/_logs/2017-11-01T16_49_11_973Z-debug.log
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Just running prettier --list-different '{src,test}/**/*.js' shows that it returns 1:

➜  redux git:(add_prettier) ✗ ./node_modules/.bin/prettier --list-different '{src,test}/**/*.js'
src/bindActionCreators.js
➜  redux git:(add_prettier) ✗ echo $?
1
Contributor

adityavohra7 commented Nov 1, 2017

Yup! Looks like it does. With this diff:

diff --git a/src/bindActionCreators.js b/src/bindActionCreators.js
index b44d6ff..6b02dfd 100644
--- a/src/bindActionCreators.js
+++ b/src/bindActionCreators.js
@@ -27,7 +27,7 @@ function bindActionCreator(actionCreator, dispatch) {
  */
 export default function bindActionCreators(actionCreators, dispatch) {
   if (typeof actionCreators === 'function') {
-    return bindActionCreator(actionCreators, dispatch)
+    return bindActionCreator(actionCreators, dispatch);
   }

   if (typeof actionCreators !== 'object' || actionCreators === null) {

Running the prepare script yields:

➜  redux git:(add_prettier) ✗ yarn prepare
yarn run v1.1.0
$ npm run clean && npm run format:check && npm run lint && npm test && npm run build

> redux@3.7.2 clean /Users/adityavohra7/Documents/Github/redux
> rimraf lib dist es coverage

> redux@3.7.2 format:check /Users/adityavohra7/Documents/Github/redux
> prettier --list-different '{src,test}/**/*.js'

src/bindActionCreators.js
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! redux@3.7.2 format:check: `prettier --list-different '{src,test}/**/*.js'`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the redux@3.7.2 format:check script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/adityavohra7/.npm/_logs/2017-11-01T16_49_11_973Z-debug.log
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Just running prettier --list-different '{src,test}/**/*.js' shows that it returns 1:

➜  redux git:(add_prettier) ✗ ./node_modules/.bin/prettier --list-different '{src,test}/**/*.js'
src/bindActionCreators.js
➜  redux git:(add_prettier) ✗ echo $?
1
@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Nov 3, 2017

Member

OK, I think this is good to go.

I do want to wait until we get next merged into master. Maintaining each branch where there are so many changes will be difficult and will break the open set of PRs. Let me work on getting them merged in and next merged as well, and then we can merge in this one without a ton of hassle.

Thanks for the hard work so far!

Member

timdorr commented Nov 3, 2017

OK, I think this is good to go.

I do want to wait until we get next merged into master. Maintaining each branch where there are so many changes will be difficult and will break the open set of PRs. Let me work on getting them merged in and next merged as well, and then we can merge in this one without a ton of hassle.

Thanks for the hard work so far!

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Nov 4, 2017

Contributor

@timdorr sounds good! Thanks for the feedback throughout the review! 😊

Contributor

adityavohra7 commented Nov 4, 2017

@timdorr sounds good! Thanks for the feedback throughout the review! 😊

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Nov 29, 2017

Member

next has been merged into master. Want to update this and get it merged in?

Member

timdorr commented Nov 29, 2017

next has been merged into master. Want to update this and get it merged in?

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Nov 29, 2017

Contributor

Sure! I'll try to update this soon! 😄

Contributor

adityavohra7 commented Nov 29, 2017

Sure! I'll try to update this soon! 😄

@adityavohra7 adityavohra7 changed the title from [chore] Added prettier via eslint to [chore] Added prettier formatting to src and test Nov 30, 2017

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Nov 30, 2017

Contributor

Updated! Let me know if you have any questions!

Contributor

adityavohra7 commented Nov 30, 2017

Updated! Let me know if you have any questions!

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Dec 8, 2017

Contributor

Bump! Let me know if there's anything I should change!

Contributor

adityavohra7 commented Dec 8, 2017

Bump! Let me know if there's anything I should change!

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Dec 9, 2017

Member

I'm good with this. @markerikson did you want to provide any input, or should we ship it?

Member

timdorr commented Dec 9, 2017

I'm good with this. @markerikson did you want to provide any input, or should we ship it?

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Dec 9, 2017

Contributor

I haven't looked at this in detail, but I have no particular complaints about running everything through Prettier.

Contributor

markerikson commented Dec 9, 2017

I haven't looked at this in detail, but I have no particular complaints about running everything through Prettier.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Dec 11, 2017

Member

Than ship it we shall. Thanks, @adityavohra7!

Member

timdorr commented Dec 11, 2017

Than ship it we shall. Thanks, @adityavohra7!

@timdorr timdorr merged commit 87071fd into reduxjs:master Dec 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adityavohra7 adityavohra7 deleted the adityavohra7:add_prettier branch Dec 11, 2017

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Dec 15, 2017

Contributor

@timdorr, thanks for merging this PR! Would you be open to a PR running prettier on examples/? I only see that conflicting with this PR.

Contributor

adityavohra7 commented Dec 15, 2017

@timdorr, thanks for merging this PR! Would you be open to a PR running prettier on examples/? I only see that conflicting with this PR.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Dec 15, 2017

Member

Those are pretty volatile, so that would probably be more trouble than it's worth. I'd also like to keep them simple to use, so additional tooling would go against that goal.

Member

timdorr commented Dec 15, 2017

Those are pretty volatile, so that would probably be more trouble than it's worth. I'd also like to keep them simple to use, so additional tooling would go against that goal.

@adityavohra7

This comment has been minimized.

Show comment
Hide comment
@adityavohra7

adityavohra7 Dec 15, 2017

Contributor

@timdorr, makes sense! Thanks once again for helping throughout this PR 😊

Contributor

adityavohra7 commented Dec 15, 2017

@timdorr, makes sense! Thanks once again for helping throughout this PR 😊

timdorr referenced this pull request in ReactTraining/react-router Jan 12, 2018

@okwolf

This comment has been minimized.

Show comment
Hide comment
@okwolf

okwolf Jan 13, 2018

Is there a specific reason for not wanting npm run format at the top level folder to also format the code in examples/? There's a lot of inconsistent formatting there that makes me reluctant to work on improving them 🤔

I've also been using prettier long enough now that it would feel weird working on code without having a way to autoformat it 😂

okwolf commented Jan 13, 2018

Is there a specific reason for not wanting npm run format at the top level folder to also format the code in examples/? There's a lot of inconsistent formatting there that makes me reluctant to work on improving them 🤔

I've also been using prettier long enough now that it would feel weird working on code without having a way to autoformat it 😂

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Jan 14, 2018

Member

Same reason we don't lint them. They're (almost) all built with Create React App and have that tooling built-in. I'd rather use the tooling from CRA than have to maintain that ourselves.

Member

timdorr commented Jan 14, 2018

Same reason we don't lint them. They're (almost) all built with Create React App and have that tooling built-in. I'd rather use the tooling from CRA than have to maintain that ourselves.

seantcoyote added a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018

tomipaul added a commit to tomipaul/redux that referenced this pull request Apr 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment