Skip to content
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

🚀 Feature: Add a git pre-push script #1243

Closed
3 tasks done
danvk opened this issue Jan 16, 2024 · 2 comments
Closed
3 tasks done

🚀 Feature: Add a git pre-push script #1243

danvk opened this issue Jan 16, 2024 · 2 comments
Labels
status: in discussion Not yet ready for implementation or a pull request type: feature New enhancement or request

Comments

@danvk
Copy link
Contributor

danvk commented Jan 16, 2024

Bug Report Checklist

  • I have tried restarting my IDE and the issue persists.
  • I have pulled the latest main branch of the repository.
  • I have searched for related issues and found none that matched my issue.

Overview

I propose that create-typescript-app set up a pre-push hook via Husky for you that runs some of the lint scripts before you push to GitHub. This will save you time and context switching when there are lint errors. It will slow down git push, but that's much more palatable (at least to me) than slowing down git commit.

Additional Info

create-typescript-app sets up a pre-commit hook that runs prettier for you. pre-commits aren't my favorite (in my mind git commit should be instant) but prettier is pretty fast so this doesn't bother me too much.

I'm finding it a bit annoying when I push to a PR branch only to find out through CI that I have a spelling mistake. Maybe spelling could run as a pre-commit when you have it enabled, but this could be a slippery slope to slow commits.

I find pre-push hooks to be a more palatable alternative. When I'm pushing to GitHub, my expectation is that I'm about to context switch anyway, so I don't mind waiting a few seconds if it's going to save me a round-trip to the GitHub Actions UI to find out about some failing linter.

Here's what I've set up in my repo:

npx husky add .husky/pre-push "pnpm pre-push"

And the script:

{
  "pre-push": "pnpm run '/^lint(?!:packages).*$/'"
}

This runs all the lint* scripts in parallel (cool!) except for lint:packages, which is kinda slow. In my repo, running this pre-push takes ~3 seconds. (eslint is the slowest step.)

So, what about adding a pre-push hook by default?

@danvk danvk added the type: feature New enhancement or request label Jan 16, 2024
@JoshuaKGoldberg
Copy link
Owner

Yeah this is something I've gone back and forth on for a while. On the one hand, it is annoying to have to push again for spelling mistakes, etc. (the spelling one is what gets me personally). But on nontrivial repos the lint step in particular can take a long time.

I personally wouldn't want this because I personally don't mind having to push again for CI. I personally tend to send draft PRs for bigger changes to use the GitHub viewer anyway. But maybe I'm in the minority here? I know @johnnyreilly likes partially different tooling setups than me.

Alternately / in addition: this might be good as an option? Opt-in or opt-out?

Leaving open in status: in discussion.

@JoshuaKGoldberg JoshuaKGoldberg added the status: in discussion Not yet ready for implementation or a pull request label Jan 16, 2024
@JoshuaKGoldberg
Copy link
Owner

Coming back to this after a while: it'll be a lot easier to enforce for a repo once there's a shared engine (#1181). Closing out for create-typescript-app itself. Cheers!

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in discussion Not yet ready for implementation or a pull request type: feature New enhancement or request
Projects
None yet
Development

No branches or pull requests

2 participants