Skip to content

Refactor Prettier Implementation (Post Commit Style) #585

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

Closed
wants to merge 2 commits into from

Conversation

jshcrowthe
Copy link
Contributor

@jshcrowthe jshcrowthe commented Mar 22, 2018

This is a follow up to #580 per a conversation with @schmidt-sebastian. This changes the behavior of the prettier implementation to do the following:

  • Functions only on changed files (.ts/.js/.json)
  • Functions post-commit and generates individual styling/licensing commits

Limitations w/ this approach:

  • No support for doing patch commits (we could hard disable them)
  • Code migrations will be stylistically nuked in this approach. (This doesn't seem to be a huge issue ATM)

Additionally we scope them to only the changed files to prevent out of scope changes
@schmidt-sebastian
Copy link
Contributor

Can we work around the limitations if we also run prettier in CI and enforce that it does not produce any diff?

* Checks if the files in question have associated diffs
*/
async function hasDiff(files) {
let { stdout } = await exec(`git diff ${files.join(' ')}`, { cwd: root });
Copy link
Contributor

Choose a reason for hiding this comment

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

You can pass --quiet and then check the exit code: https://git-scm.com/docs/git-diff.

You can then likely skip passing in the filenames, which will prevent me from breaking this in the future by maliciously using filenames that contain single quotes.

@schmidt-sebastian
Copy link
Contributor

Ping :)

@mikelehen
Copy link
Contributor

I'm not super excited about doing this on commit. Last I talked to Josh I think he was going to look into making the existing pre-push hook only run prettier on changed files which should solve the original performance issues (and spurious prettier edits on files you didn't touch).

@mikelehen mikelehen removed their assignment Apr 9, 2018
@jshcrowthe jshcrowthe closed this May 21, 2018
@jshcrowthe
Copy link
Contributor Author

I'ma close this for now. Will revisit in the future.

@firebase firebase locked and limited conversation to collaborators Oct 22, 2019
@hsubox76 hsubox76 deleted the post-commit branch May 2, 2024 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants