-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Also remove pre-push hook? #556
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
Comments
If you're confident that they're correct can't you just use But one of the most common forms of CI build failures is formatting, so I'd like to at a minimum keep that. That at least saves time by not having to re-review PRs and such. |
Yes, but it's a pain to forget it.
Interesting: from a reviewer/maintainer perspective, do you re-review when a build is failing? I'll try to send a PR soon to remove typecheck and linting, thanks! |
If we approve a PR, and then a new commit comes in (that's not a straight merge commit), github dismisses the review (because it's stale - the code has changed). So if we approve then the user commits to fix a formatting issue, it'll dismiss the review and we'll have to be conscious of it and re-approve. That or we request changes then have to approve once they fix the formatting, which is a bit cumbersome too. Having it checked pre-push helps guard against people that "fire and forget" their PRs and only update them once they're commented on. |
Following #259: it's inconvenient to wait for a full lint + typecheck after every
git push
. CI builds reinforce them anyway. Can they be removed, please?The text was updated successfully, but these errors were encountered: