-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
MAINT: Chain some exceptions. #16418
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
Thanks for the patch! Everything looks great with the PR, just looks like I was unclear about how to choose between the two options in the tracking issue. |
numpy.ctypes
and numpy.core
numpy.ctypes
and numpy.core
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.
Just a style nit. It might also be nice to convert the strings in AttributeError
s in numpy/core/records.py
to f-strings (this might get the one suggestion below the line limit on its own) or to use .format
, but it's not necessary.
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Ok, I've implemented the feedback I've gotten. I also found some other places where I could fix the exceptions like this, should I just wait for this to be merged or should I keep working on this branch? |
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.
LGTM - I'd say hold off on new modifications (i.e. adding more chained exceptions) so the review doesn't get too muddled.
Thanks @zaak71 |
This is related to issue #15986. I have modified a few files in the numpy and numpy/core folders to fit what has been requested here. This is my first PR on GitHub so if I did anything wrong please let me know. Thank you.