-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
MAINT: Raise different type of errors #20389
Conversation
Please check your indentation |
The conda build failure is expected. |
Thanks @andrekwr! Some of those The reason for catching and reraising the if isinstance(fname, os_PathLike):
fname = os_fspath(fname)
if isinstance(fname, str):
fid = np.lib._datasource.open(fname, 'rt', encoding=encoding)
fid_ctx = contextlib.closing(fid)
else:
# If here, we expect fname to be a sequence or iterator of strings.
fid = fname
fid_ctx = contextlib.nullcontext(fid)
fhd = iter(fid) For many cases, bad input to this code will result in an exception that gives a reasonable error message to the user without having to reraise an exception. Some examples:
The error messages that are not very helpful occur when the first argument is not a string or a path, and also not an iterator, such as an integer or
So I think the only code that needs to be wrapped in a if isinstance(fname, os_PathLike):
fname = os_fspath(fname)
if isinstance(fname, str):
fid = np.lib._datasource.open(fname, 'rt', encoding=encoding)
fid_ctx = contextlib.closing(fid)
else:
# If here, we expect fname to be a sequence or iterator of strings.
fid = fname
fid_ctx = contextlib.nullcontext(fid)
try:
fhd = iter(fid)
except TypeError as e:
raise TypeError(
"fname must be a string, a filehandle, a sequence of strings,\n"
f"or an iterator of strings. Got {type(fname)} instead."
) from e Note that I did some copy-editing in the error message of the reraised
We should add a unit test for this behavior. The tests for def test_bad_fname(self):
with pytest.raises(TypeError, match='fname must be a string,'):
np.genfromtxt(123) |
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.
(Github requires a comment here before I can submit the review with "Request changes" clicked. The requested changes are in my previous comment.)
Hey @WarrenWeckesser , sorry for the delay. |
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 @andrekwr.
There are a few whitespace issues that I'll fix, and then I'll merge.
Hello,
I added the errors messages as proposed by @WarrenWeckesser in #20329.