-
Notifications
You must be signed in to change notification settings - Fork 370
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
better disallowmissing error message #2966
Conversation
doctests failure is unrelated. I fix it in #2967. |
Thanks. I don't use column numbers much so a name would be more familiar to me. Maybe reporting a row too would be useful? I'm not sure. |
Fixed. Is it better now? |
Cheers. Closes #2965 |
if any(ismissing, x) | ||
y = x | ||
else | ||
y = disallowmissing(x) | ||
end | ||
end |
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.
Instead of this, we could just do error || continue
in the if row !== nothing
branch above.
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 could not do continue
because we still need to execute line 2088, so instead we would need to write inside if row !== nothing
:
if error
col = _names(df)[i]
throw(ArgumentError("Missing value found in column :$col in row $row"))
else
y = x
end
Do you think it would be simpler to understand by the potential readers of the code?
I used this explicit logic because when I read the old condition:
if !error && Missing <: eltype(x) && any(ismissing, x)
I had to spend some time to make sure it was OK (although I think I have written it some time ago).
Therefore I would vote for leaving things as they are as it is I think easier to understand if some user wants to inspect the code.
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.
OK. But I'd use try... catch
when !error
too.
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 have changed disallowmissing
and disallowmissing!
the way that I think you wanted.
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.
OK. But I'd use try... catch
when !error
too.
Thank you! |
Improve the error message in case column conversion fails, e.g.: