-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
TST: Add hook for Disallow bare pytest.raises #38799
TST: Add hook for Disallow bare pytest.raises #38799
Conversation
bf916d4
to
c01011a
Compare
c01011a
to
228aa76
Compare
.pre-commit-config.yaml
Outdated
entry: python scripts/validate_unwanted_patterns.py --validation-type="bare_pytest_raises" | ||
types: [python] | ||
files: ^pandas/tests/ | ||
exclude: ^pandas/tests/(arrays|computation|dtypes|extension|indexes|indexing|io|libs|reductions|resample|reshape|series|window)/ |
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.
If you can wait a day or two we can probably get this down to the much shorter list
^pandas/tests/(computation|extension|indexing|io|series)/
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.
sure - or, we could get this is now, and then we each PR you submit you narrow it down further?
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.
There are 4 open PRs: #38720, #38800, #38804, #38805. If we edit this line in each of them they will all have merge conflicts with each other.
After those 4 are closed, I am happy to get this one merged and address this line in each future PR, because what's left are the ones that aren't super straightforward and I might need some help with them.
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.
yeah i agree, let's wait on this for a little
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.
sure, makes sense - have updated and it's much shorter now, thanks!
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.
We are now down to just extension and can merge this, and imo close #30999
looks reasonable, can you rebase once more |
thanks @MarcoGorelli |
xref #30999
Adding a hook with lots of excluded folders (for now), this can be made stricter as we go along, there's not that much left