Add git hooks and simplify scripts #8427

lipis · 2018-09-21T16:56:20Z

Add git hooks to make sure the files are formatted before commiting
Simplify format* scripts
Drop prettier-eslint (is just a wrapper for ESLint and Prettier that we are using already)

The actual formats will happen in follow up PRs

www/src/data/StarterShowcase/generatedGithubData/gatsby-advanced-blog.json

pieh · 2018-09-25T12:28:57Z

I think we would need to consolidate husky things - we do have it setup for www #8229 right now, (but at least for me - this setup in www works globally for repo - so not sure if it's my issue or just how it works) - we probably would want husky setup in single place - repo main package.json

DSchau · 2018-09-25T14:34:08Z

@pieh I like the idea of using something like Husky and I completely agree that it should be at the root of the repo. It seems like a bug or something that the one at www/ level is applying to all files.

I'd rather be explicit and define exactly what files we auto format.

DSchau

@lipis I think we're very much in favor of the intent of this PR.

Would you be able to separate the functionality from the formatting? It's kinda challenging to review a PR with 1000 files changed 👀, so it'd be great to take a look at just the functionality, then introduce the formatting in a subsequent PR.

It'd also be nice if the build scripts in package.json were tweaked minimally, but I would need to take another look at exactly what's changed and why it was changed (e.g. prettier-eslint -> eslint)

Sound like a plan?

jquense · 2018-09-25T14:50:35Z

package.json

-    "format-yaml": "prettier --write \"**/*.y?(a)ml\"",
+    "format": "npm-run-all -p format-packages format:js format:other",
+    "format:other": "yarn prettier --write",
+    "format:js": "eslint --ignore-path .gitignore \"**/*.{js,jsx}\" --fix",


why specify the the ignore path here?

Because otherwise it will try to parse more files as far as I know if it's not included..

also let's put more ignored directories (in another file) instead of specifying which ones to format... there is no need to have 10 different format rules, as it's the same project.

I'm open for better names here

package.json

DSchau · 2018-09-25T14:52:40Z

@lipis also you can ignore the commit hooks with git commit --no-verify if that helps!

lipis · 2018-09-25T14:58:25Z

I'll do that..

.eslintrc.json

.prettierrc

package.json

Changes to PR!

www/package.json

lipis · 2018-09-25T15:07:49Z

I updated the PR with only the changes and simplified package.json.

As a first step we can run the format:other to format all the files except js

DSchau · 2018-09-25T15:08:47Z

package.json

-    "format-integration": "prettier-eslint --write \"integration-tests/**/src/**/*.js\" \"integration-tests/**/cypress/**/*.js\"",
-    "format-yaml": "prettier --write \"**/*.y?(a)ml\"",
+    "format": "npm-run-all -p format-packages format:js format:other",
+    "format:other": "yarn prettier --write",


Can we do npm run prettier --write here? I'd like to minimize the amount of places we require yarn explicitly!

I would say we could get rid of npm-run-all and use yarn everywhere since we have yarn.lock and no package-lock.json

I disagree a lot with that! We want to keep barrier to entry low, so a change like that just makes that barrier higher and harder to get to for new contributors.

That's invisible to the users and we already using yarn in the 'prebootstrap' and defined as engine..

I don't think the engine is prohibiting anyone from contributing who uses npm. I agree that it's an "invisible" change (i.e. not defined as a commit hook or anything, yet), but I don't see the benefit of changing that out here?

I can imagine a user running npm run format:other or npm run format and being confused that fails. In that scenario, we made it harder for that user to contribute, and I'm not sure if it was for a real benefit.

lipis · 2018-09-25T15:11:06Z

Here is the PR for formatting all files except JS (just as an example)
lipis#1

.eslintrc.json

lipis · 2018-09-25T17:21:42Z

I updated the scripts.. in the next PRs we can format and fix the linting errors.

* 'master' of github.com:gatsbyjs/gatsby: Category typos fixes and consolidation (gatsbyjs#8520) debugging build: error due to import/require mix (gatsbyjs#8493) updating name Adding pagination docs: add pagination doc (gatsbyjs#8476)

NickyMeuleman · 2018-09-25T17:58:51Z

It looks like you plan to use eslint-plugin-prettier to integrate Prettier into the ESLint config (to make ESLint format .js files with Prettier) and are using eslint-config-prettier to prevent issues where the 2 tools want to do something else.

This is great! 👍
I wrote about how to wire that up

…ate if we want error

even if `public` dirs are in .prettierignore, prettier will construct settings for each .json file before discarding them if they are in .prettierignore. This caused pretty bad prettier performance as it was doing that for every query result file (especially bad if you tried running benchmark pages with 50000 pages as prettier would evaluate 50000 json files

lipis · 2018-11-06T18:30:32Z

scripts/on-lint-error.js

+  If you want disable this hook for all future commits:
+    npm run hooks:uninstall
+  `
+)


lipis · 2018-11-06T18:32:54Z

@pieh Why not prettify the JSON files as well.. and add to the ignore list if they are generated automatically?

pieh · 2018-11-06T18:33:51Z

I missed weekend deadline as I needed to consult with some people working on www PRs (if resolving conflicts after running massive autoformat wouldn't cause them too much trouble).

So few things I just pushed here:

updated ignore rules to cover more things we don't want lint/format
removed .json linting/formatting (at least for now) - see commit msg for reasoning 031ec68
updated lint-staged to version 8 (it adds support to lint/format partially staged files, so any changes that you don't actually commit don't need to pass linting)
added hooks:(un)install scripts to help uninstall and re-install hooks
and added not rejection (thanks @NickyMeuleman for help :) ) - this is how it looks like (message was changed a bit from when I took the screenshot):

pieh · 2018-11-06T18:36:09Z

@pieh Why not prettify the JSON files as well.. and add to the ignore list if they are generated automatically?

See 031ec68 for explanation - yes they won't be linted/formatted, but prettier will prepare configuration for every .json (in this instance) file and it was slowing linting/formatting (i was stuck on format:other for 15 minutes because I had 50000 json files that were ignored in one of our benchmark sites and it didn't even finish).

I didn't want to spend/waste more time on trying to figure it out - would prefer to merge this and if possible to fix that problem in follow up PR

lipis · 2018-11-06T18:44:29Z

I also updated the #8622 and #8623

lipis · 2018-11-06T18:45:43Z

I didn't want to spend/waste more time on trying to figure it out - would prefer to merge this and if possible to fix that problem in follow up PR

Makes sense.. we can do it in the follow up!

pieh · 2018-11-06T18:47:52Z

I also updated the #8622 and #8623

Did you run "just" format on them or did you also fix manually any errors that can't be autofixed? I have branch master...pieh:hooks-plus-format where I did autoformatting (both code and other) + manual fixes to get linting pass with those changes

* 'master' of github.com:gatsbyjs/gatsby: (63 commits) Update how-to-contribute.md to mention the style guide when writing blogs/tutorials (gatsbyjs#9742) Added Tylermcginnis website (gatsbyjs#9619) Fix grammar and punctuation (gatsbyjs#9498) Fix typo of plugin authoring (gatsbyjs#9737) Authentication tutorial - small fixes (gatsbyjs#9738) chore: move run-sift (gatsbyjs#9549) docs: fix minor typo (gatsbyjs#9730) chore(release): Publish fix(gatsby-plugin-page-creator): ensure that __tests__ directory is actually ignored (gatsbyjs#9720) fix: revert admin redirect (gatsbyjs#9728) fix: adjust page order to make nested matchPaths work (gatsbyjs#9719) feat(gatsby-plugin-sharp): cache base64 if possible (gatsbyjs#9059) chore(release): Publish fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources (gatsbyjs#9679) chore(release): Publish fix: ensure babel-preset-gatsby can be used with unit tests (gatsbyjs#9629) feat(www): Filter posts by date (gatsbyjs#9400) fix(blog): azure blog post url date (gatsbyjs#9718) feat(blog): Add post on publishing to Azure (gatsbyjs#8868) Emphasize importance of promise return on source-plugin docs (gatsbyjs#9650) ...

lipis · 2018-11-06T18:51:45Z

I just run the code without any manual work.. it's just to see what is being affected and what is needed to be fixed.

I'll fix the errors and do manual updates when we merge the hooks.

pieh · 2018-11-06T18:55:47Z

I just run the code without any manual work.. it's just to see what is being affected and what is needed to be fixed.

I'll fix the errors and do manual updates when we merge the hooks.

Let's merge this PR, then 2 of your (if they will apply cleanly) and then we can merge remaining of manual fixes that I already did from my branch (no sense to add more work for You if it's already done)

lipis · 2018-11-06T19:05:24Z

Let's merge this PR, then 2 of your (if they will apply cleanly) and then we can merge remaining of manual fixes that I already did from my branch (no sense to add more work for You if it's already done)

Let's press the buttons! After merging this.. I will update my other PRs and force push the changes.. and then you can apply your changes..!!

Looking forward to pass all the tests!

pieh · 2018-11-06T19:11:39Z

eta ~15 minutes for merge - just gave final "warning" for rest of team to get any changes that are complete merged (to avoid unneeded conflict resolutions for PRs that were ready but for some reason not merged).

https://www.youtube.com/watch?v=8V9TCD0TTtk ;)

pieh · 2018-11-06T19:31:30Z

@lipis merged, can you update other PRs so they don't include same changes as this one (git is weird sometimes)

lipis · 2018-11-06T19:33:25Z

Yep! Just did.. :)

After we merge #8427

- [x] Add git hooks to make sure the files are formatted before commiting - [x] Simplify `format*` scripts - [x] Drop prettier-eslint (is just a wrapper for ESLint and Prettier that we are using already) ---- ### The actual formats will happen in follow up PRs - [Format preview for `format:code`](https://github.com/gatsbyjs/gatsby/pull/8623/files) - [Format preview for `format:other`](https://github.com/gatsbyjs/gatsby/pull/8622/files)

After we merge gatsbyjs#8427

lipis commented Sep 21, 2018

View reviewed changes

www/src/data/StarterShowcase/generatedGithubData/gatsby-advanced-blog.json Outdated Show resolved Hide resolved

lipis requested a review from a team as a code owner September 25, 2018 14:44

lipis requested a review from a team September 25, 2018 14:44

lipis requested a review from a team as a code owner September 25, 2018 14:44

DSchau previously requested changes Sep 25, 2018

View reviewed changes

jquense reviewed Sep 25, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

Refactor package.json and add git hooks

12974b5

lipis force-pushed the hooks branch from 5111322 to 12974b5 Compare September 25, 2018 15:02

lipis commented Sep 25, 2018

View reviewed changes

.eslintrc.json Show resolved Hide resolved

lipis commented Sep 25, 2018

View reviewed changes

.prettierrc Show resolved Hide resolved

lipis commented Sep 25, 2018

View reviewed changes

package.json Show resolved Hide resolved

lipis commented Sep 25, 2018

View reviewed changes

www/package.json Show resolved Hide resolved

DSchau reviewed Sep 25, 2018

View reviewed changes

pieh reviewed Sep 25, 2018

View reviewed changes

.eslintrc.json Show resolved Hide resolved

lipis added 5 commits September 25, 2018 19:32

Npm

c898599

Lint JS

7cdbe76

Ignore

1a00df7

Simpler

49a4d5e

Staged

d85b10c

pieh added 8 commits November 6, 2018 13:03

add cy and Cypress globals

7450f6c

warn for react/no-unescaped-entities instead of error - need to evalu…

9539a8b

…ate if we want error

ignore .cache dirs

8ad560a

ignore specimen files

96715be

format css

37f7a40

adjust lint-staged rule to not apply to json and apply to css

9257ee9

don't reject commits and provide some feedback message

2699831

lipis commented Nov 6, 2018

View reviewed changes

scripts/on-lint-error.js

If you want disable this hook for all future commits:

npm run hooks:uninstall

`

)

Copy link

Contributor Author

lipis Nov 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

Add EOL

2f6a57f

Rename scripts from *:js to *:code

c294bda

pieh approved these changes Nov 6, 2018

View reviewed changes

pieh merged commit 9cc8f4d into gatsbyjs:master Nov 6, 2018

pieh pushed a commit that referenced this pull request Nov 6, 2018

chore: Format JS Files (#8623)

e1f1a98

After we merge #8427

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019

chore: Format JS Files (gatsbyjs#8623)

a3cb916

After we merge gatsbyjs#8427

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023

chore: Format JS Files (gatsbyjs#8623)

90d9b42

After we merge gatsbyjs#8427

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add git hooks and simplify scripts #8427

Add git hooks and simplify scripts #8427

lipis commented Sep 21, 2018 •

edited

Loading

pieh commented Sep 25, 2018

DSchau commented Sep 25, 2018

DSchau left a comment

jquense Sep 25, 2018

lipis Sep 25, 2018

lipis Sep 25, 2018

lipis Sep 25, 2018

DSchau commented Sep 25, 2018

lipis commented Sep 25, 2018

lipis commented Sep 25, 2018

DSchau Sep 25, 2018

lipis Sep 25, 2018

DSchau Sep 25, 2018

lipis Sep 25, 2018

DSchau Sep 25, 2018

DSchau Sep 25, 2018 •

edited

Loading

lipis Sep 25, 2018

lipis commented Sep 25, 2018 •

edited

Loading

lipis commented Sep 25, 2018

NickyMeuleman commented Sep 25, 2018

lipis Nov 6, 2018

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018

pieh commented Nov 6, 2018 •

edited

Loading

lipis commented Nov 6, 2018

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018 •

edited

Loading

pieh commented Nov 6, 2018

lipis commented Nov 6, 2018

Add git hooks and simplify scripts #8427

Add git hooks and simplify scripts #8427

Conversation

lipis commented Sep 21, 2018 • edited Loading

The actual formats will happen in follow up PRs

pieh commented Sep 25, 2018

DSchau commented Sep 25, 2018

DSchau left a 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

DSchau commented Sep 25, 2018

lipis commented Sep 25, 2018

lipis commented Sep 25, 2018

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

DSchau Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipis commented Sep 25, 2018 • edited Loading

lipis commented Sep 25, 2018

NickyMeuleman commented Sep 25, 2018

Choose a reason for hiding this comment

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018

pieh commented Nov 6, 2018 • edited Loading

lipis commented Nov 6, 2018

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018

lipis commented Nov 6, 2018

pieh commented Nov 6, 2018 • edited Loading

pieh commented Nov 6, 2018

lipis commented Nov 6, 2018

lipis commented Sep 21, 2018 •

edited

Loading

DSchau Sep 25, 2018 •

edited

Loading

lipis commented Sep 25, 2018 •

edited

Loading

pieh commented Nov 6, 2018 •

edited

Loading

pieh commented Nov 6, 2018 •

edited

Loading