-
-
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
Manual doctests (part 4) #20224
Manual doctests (part 4) #20224
Conversation
6d0b9ed
to
e8f84d3
Compare
Added backtraces again, need to decide what should be done with the |
@@ -241,23 +240,18 @@ julia> g(2, 3.0) | |||
8.0 | |||
|
|||
julia> g(2.0, 3.0) | |||
ERROR: MethodError: g(::Float64, ::Float64) is ambiguous. Candidates: | |||
g(x, y::Float64) in Main at none:1 | |||
g(x::Float64, y) in Main at none:1 |
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're showing the closest candidates in most of the others, right?
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.
Yes. But it cannot be done for this one, it will show closest candidates which are defined in the sandbox module which the test is run in. So either we don't doctest this or we leave out the closes candidates.
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.
okay fair enough - so normal doctests run in Main but named ones don't?
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.
They are run in Main, but all functions are defined in its own module. I think... These two tests both pass at least:
```jldoctest
julia> current_module()
Main
```
```jldoctest currentmod
julia> current_module()
Main
```
Note that this is the only one where this have been a problem. Perhaps we can skip the stacktrace for this one? The text section right after explains what is going on as well.
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
julia> Foo((), 23.5, 1) | ||
ERROR: InexactError() | ||
in convert(::Type{Int64}, ::Float64) at ./float.jl:656 |
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.
leave in for now?
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.
Just missed to add back this one, will update.
@@ -624,7 +621,7 @@ arguments to the object constructor. | |||
Since the type `Point{Float64}` is a concrete type equivalent to `Point` declared with [`Float64`](@ref) |
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.
there's some nonstandard indentation in the two norm
examples in the collapsed block here, I'll get them at some point if you don't
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.
Sure, I will fix that when I update this PR
|
||
julia> Point{Float64}(1.0,2.0,3.0) | ||
ERROR: MethodError: no method matching Point{Float64}(::Float64, ::Float64, ::Float64) | ||
Closest candidates are: | ||
Point{Float64}{T}(::Any, ::Any) at none:3 | ||
Point{Float64}{T}(::Any) at sysimg.jl:66 |
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.
keep candidates?
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.
IIRC there were none shown for this example. Note that there are no [...]
added.
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.
wonder if that's an intentional change?
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 looks like it might be a regression. Not sure yet, but maybe the type system rewrite? I'd be surprised if it were intended but might be wrong
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.
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.
ref #20292
julia> f(2.0, 3) | ||
ERROR: MethodError: no method matching f(::Float64, ::Int64) | ||
Closest candidates are: | ||
f(::Float64, !Matched::Float64) at none:1 | ||
... | ||
f(::Float64, ::Float64) at none:1 |
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.
ah right this is waiting on a color / !Matched
decision
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.
Yes.
e8f84d3
to
a0663bd
Compare
Added |
Lets keep track of #20292 and update this when/if that issue is resolved, then this is good to go? That fix might also affect other doctests too. |
Eeh I might have missunderstood the last comment and thumbs up.. Anyway we can fix it when the issue get resolved. |
You did what I meant 😄 |
julia> Point(1,2.5) | ||
ERROR: MethodError: no method matching Point{T}(::Int64, ::Float64) | ||
... | ||
ERROR: MethodError: no method matching Point(::Int64, ::Float64) |
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 also wonder whether the dropping of the {T}
here was an intentional change or not
Pretty straight forward commits, with a reservation on what is decided regarding showing backtraces (ref. #20202 (comment))
(
make check-whitespace
and the doctests pass locally, remove[ci skip]
from commit message if/when this is merged)