-
-
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
Extended printing at a MethodError. #9485
Conversation
Just tested this and it works great :) Good work! I'm against including the file and line number, it makes the output to verbose. Also the user can get the information using |
@jhasse I do agree with you it was very verbose so have removed the printing of file and line number again and removed the misplaced space you found. If nobody have any further concerns then it is ready to merge. |
I have refactored the pull-request to use |
5da9faa
to
e2099de
Compare
@tkelman could you help me figure out the Travis failure? I have seen the failure on mac and windows but not on linux. I cannot reproduce it locally. The error is:
The error goes away if i wrap the function difinitions in a module like:
And of cause reference to the function in the module by |
What I do not understand is why the line with |
Seems like it might be a variable named |
… match and removed the requirement of equal lengths of args. The match also takes into account that parametric methods args needs to have the same type for one tvar using typeintersect. There is also added test of `show_method_candidates`.
e2099de
to
25f18f6
Compare
Thank you @ivarne. There where a |
I have squashed the commits, added test cases and made Travis happy. From my point of view this is ready to merge unless people have some further feedback. |
i != 1 && print(buf, ", ") | ||
# If isvarargtype then it checks wether the rest of the input arguements matches | ||
# the varargtype | ||
j = Base.isvarargtype(method.sig[i]) ? length(t_i) : i |
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.
weird indentation here
This looks awesome---I've really been enjoying #7714, and this looks to make it even better. One thing to consider is the use of the new |
I'm against that. IMHO color should always be available and if it isn't (e. |
Can you clarify what you mean by "it should be fixed"? Some terminals simply don't support color. I should be explicit about my underlying motivation for suggesting this: I'm not certain, but I suspect our travis tests run with |
I have made the small correction according to you @timholy. I agree with @jhasse that it could be misleading with all upper cases. Could we do something like putting
Or
|
I especially like the |
I mean that these terminals should be fixed to support colors. "Color
I see. What is the reason behind deactivating colors for tests? |
julia is asking the OS whether color is available, and if the answer is "no" then it doesn't try to use it. For example, when you redirect STDOUT to a file, odds are you don't want all those dratted special characters appearing in the output. When you run your tests as |
@timholy then I'm going to implement that suggestion tomorrow when color is not supported by the terminal. |
What do you mean by internal display? |
4e4315f
to
4c13083
Compare
… when `have_color=false`
6202087
to
62ae4e5
Compare
@timholy I have updated the pull-request to print this when
I did not do as we discussed yesterday due to
And hence I do not know what should be in front of |
|
Extended printing at a MethodError.
Thanks for doing this! |
I have extended #7714 with the following:
The following will demo the difference, suppose you have the following methods deffined:
Before you would get:
Where it shows that the parametric type matches all arguments and the second is two arguments from matching but in reality if you change the first argument to an Int then it matches. With this pull-request it displays:
@jhasse could you approve/comment on my changes since you are the original author of this, nice work and idea by the way. Also pinging some of the reviewers in #7714 to hear their opinion, @ivarne, @simonster, @mbauman.