Skip to content

Adding Prettier on Commit #580

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Adding Prettier on Commit #580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
],
"scripts": {
"dev": "lerna run --parallel --scope @firebase/* --scope firebase dev",
"precommit": "lint-staged",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh boy... I don't know if I trust prettier enough to let it silently modify my commits. Then I can't tell what it did and I might not notice problems. Today I always run "yarn prepush" manually, check what it did, and then if I'm satisfied, I'll clean up the git history via "git rebase -i master" or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the staged files makes it really easy to figure out what files to re-format. A while ago, I tried something similar to what you suggested in #531 (diff against origin & upstream) but wasn't able to get it to work properly in between my other work.

I personally don't like the Prettier commits but I also don't want to take them away from you (since I can just rebase if need be). Maybe there is some way we can still stage them in a separate commit (by not passing --write, or not doing git add?). Do you have suggestions for this, @jshcrowthe?

I do want to ask though if it is even possible to disagree with prettier. If you manually re-format prettier's changes, doesn't it just revert these changes on the next run?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what this would entail, but we could do a post-commit hook and just try and format the last diff? I dunno what it would require/look like, but it seems doable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I sometimes choose to wrap lines differently (or restructure code to avoid wrapping, etc.). And I just like to know what prettier is doing. We've run into cases where it just does wonky reformatting (either due to bugs or poor design) and I like to notice those when they happen.

"prepare": "lerna run --scope @firebase/* --scope firebase prepare",
"prepush": "node tools/gitHooks/prepush.js",
"link:packages": "lerna exec --scope @firebase/* --scope firebase -- yarn link",
Expand Down Expand Up @@ -52,6 +53,7 @@
"istanbul-instrumenter-loader": "^3.0.0",
"lcov-result-merger": "^2.0.0",
"lerna": "^2.9.0",
"lint-staged": "^7.0.0",
"listr": "^0.13.0",
"merge2": "^1.2.1",
"mkdirp": "^0.5.1",
Expand All @@ -62,5 +64,15 @@
"semver": "^5.5.0",
"simple-git": "^1.91.0",
"yargs": "^11.0.0"
},
"lint-staged": {
"*.ts": [
"prettier --write",
Copy link
Contributor

Choose a reason for hiding this comment

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

prettier.js references repo-root/.prettierrc. Are we losing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just verified that this still works.

"git add"
],
"*.js": [
"prettier --write",
"git add"
]
}
}
4 changes: 0 additions & 4 deletions tools/gitHooks/prepush.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

const { doPrettierCommit } = require('./prettier');
const { doLicenseCommit } = require('./license');
const { resolve } = require('path');
const simpleGit = require('simple-git/promise');
Expand Down Expand Up @@ -43,9 +42,6 @@ $ git stash pop
return process.exit(1);
}

// Style the code
await doPrettierCommit();

// Validate License headers exist
await doLicenseCommit();

Expand Down
60 changes: 0 additions & 60 deletions tools/gitHooks/prettier.js
View file
Open in desktop

This file was deleted.

Loading