-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Update printing for length and size method error #25339
Update printing for length and size method error #25339
Conversation
base/replutil.jl
Outdated
@@ -380,6 +380,10 @@ function showerror(io::IO, ex::MethodError) | |||
end | |||
print(io, ")") | |||
end | |||
if f_is_function && ( name == :length || name == :size ) |
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.
This should check whether the type that was attempted to be called is an iterator (e.g. using applicable(start, ex.args)
) and iteratorsize is HasLength
. The error message should then suggest changing the iteratorsize trait or implementing the methods.
Also needs a test case. Thanks for working on this 👍 |
Thanks for the comments. For the test case, should we read standard out to ensure that this error is being printed? |
base/replutil.jl
Outdated
@@ -380,6 +380,15 @@ function showerror(io::IO, ex::MethodError) | |||
end | |||
print(io, ")") | |||
end | |||
if f_is_function && applicable(start, ex.args) && !method_exists(f, arg_types) | |||
if Base.iteratorsize(ex.args) == Base.HasLength() | |||
print(io, "\nYou may consider implementing an additional", |
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'd just say ... implementing the `length` method
. Same below (no need to say "such as", as these are exactly the methods to implement).
Thanks @nalimilan for the comments. @Keno or others, should embedding-test.jl execute this block and print this error message? If not, what condition would have to be used in this block to prevent the stderr in embedding-test.jl to have this statement? |
@Keno, I believe this is ready to be reviewed. Let me know if you think other changes should be made. I can also rebase the commits into a single or a couple of commits. |
Change condition to use `applicable` based on comments by @Keno
Based on comments from @nalimilan
Updates/Closes #25339, using the new register_error_hint ability Fixes #25232 Co-authored-by: Jameson Nash <[email protected]>
Fixes #25232
This commit adds special printing that hints that the user might be missing some of the additional iterator methods such as
length
andsize
.