Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 3 commits into from
Jun 4, 2020
Merged

MAINT: Chain some exceptions. #16418

merged 3 commits into from
Jun 4, 2020

Conversation

zaak71
Copy link
Contributor

@zaak71 zaak71 commented May 28, 2020

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.

numpy/core/_dtype.py Outdated Show resolved Hide resolved
numpy/ctypeslib.py Outdated Show resolved Hide resolved
@eric-wieser
Copy link
Member

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.

@charris charris changed the title ENH: Chain extensions in numpy and numpy/core ENH: Chain exceptions in numpy and numpy/core May 29, 2020
@charris charris changed the title ENH: Chain exceptions in numpy and numpy/core MAINT: Chain exceptions in numpy and numpy/core May 29, 2020
@charris charris changed the title MAINT: Chain exceptions in numpy and numpy/core MAINT: Chain some exceptions in numpy.ctypes and numpy.core May 29, 2020
@charris charris changed the title MAINT: Chain some exceptions in numpy.ctypes and numpy.core MAINT: Chain some exceptions. May 29, 2020
Copy link
Contributor

@rossbar rossbar left a 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 AttributeErrors 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.

numpy/core/records.py Outdated Show resolved Hide resolved
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
@zaak71
Copy link
Contributor Author

zaak71 commented May 30, 2020

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?

Copy link
Contributor

@rossbar rossbar left a 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.

@mattip mattip merged commit 489de42 into numpy:master Jun 4, 2020
@mattip
Copy link
Member

mattip commented Jun 4, 2020

Thanks @zaak71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.