-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref: Remove JS tests from precommit #29155
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
Conversation
Those tests were added because people kept forgetting updating snapshot files. However, we don't have those anymore. Meanwhile backend devs see the entire js testsuite executing when they merge master into their branch This relates to, but doesn't fully solve #25291, as JS lints still run on `git merge master`. In the longer-term it might be best to move everything to pre-commit.yml which doesn't do that.
| if py: | ||
| # python autoformatting is now done via pre-commit (black) | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see an obvious reason as to why if x: pass is something worthwhile to keep in code...
armenzg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this!
joshuarli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, now i don't have to rm .git/hooks/pre-commit.legacy anymore
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
flub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason this stalled?
…-from-lint-engine
|
@flub yes, two reasons: flaky CI and short attention span :P |
Those tests were added because people kept forgetting updating snapshot
files. However, we don't have those anymore.
Meanwhile backend devs see the entire js testsuite executing when they
merge master into their branch
This relates to, but doesn't fully solve
#25291, as JS lints still run
on
git merge master. In the longer-term it might be best to moveeverything to pre-commit.yml which doesn't do that.