Skip to content
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

display_error should call latest showerror, pt 2 #20497

Merged
merged 2 commits into from
Feb 14, 2017
Merged

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Feb 7, 2017

Ref https://discourse.julialang.org/t/custom-showerror-doesnt-always-trigger/1937 #19916 #19864

Does this warrant a test that spawns a new Julia process? Can't replicate it otherwise.

Also, @stevengj, is there a reason you used the following syntax:

eval(Main, Expr(:body, Expr(:return, Expr(:call, Base.display_error,
                                          errio, QuoteNode(val), bt))))

while eval(Expr(:call, display_error, errio, val, bt)) seems to work as well?

@maleadt maleadt added the bugfix This change fixes an existing bug label Feb 7, 2017
@yuyichao
Copy link
Contributor

yuyichao commented Feb 7, 2017

while eval(Expr(:call, display_error, errio, val, bt)) seems to work as well?

The body and return doesn't seem necessary but you should at least wrap the val in a QuoteNode.

@vtjnash
Copy link
Member

vtjnash commented Feb 7, 2017

QuoteNode won't pass through eval correctly unless it is also wrapped in body and return (it'll get wrapped in those anyways, but a copy of the contents of the quote-node will get made / changed in some cases)

@maleadt
Copy link
Member Author

maleadt commented Feb 7, 2017

Hmm, seems like I'm misunderstanding what QuoteNode is needed for.
Isn't val a plain value (as it was used before), and why does it need to be quoted?

I only tested that short :call for my changes in client.jl, does it need QuoteNode over there as well?

@yuyichao
Copy link
Contributor

yuyichao commented Feb 7, 2017

Because you are allowed (though not recommended) to throw an special AST object.

@stevengj
Copy link
Member

stevengj commented Feb 7, 2017

I really wish we had invokelatest to avoid this kind of tricky eval call...

@vtjnash
Copy link
Member

vtjnash commented Feb 7, 2017

Yeah, I hereby withdraw my objection to adding invokelatest for v0.6.

@ararslan ararslan added the display and printing Aesthetics and correctness of printed representations of objects. label Feb 7, 2017
maleadt added a commit that referenced this pull request Feb 13, 2017
test/spawn.jl Outdated
@@ -458,3 +458,7 @@ end

# readlines(::Cmd), accidentally broken in #20203
@test sort(readlines(`$lscmd -A`)) == sort(readdir())

# issue #19864 (PR #20497)
@test readchomp(pipeline(ignorestatus(`$exename --startup-file=no -e "type Error19864 <: Exception; end; Base.showerror(io::IO, e::Error19864) = print(io, \"correct19864\"); throw(Error19864())"`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can wrap inside the -e code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also use struct now

@maleadt
Copy link
Member Author

maleadt commented Feb 13, 2017

Rebased, added test and addressed review comments.

@maleadt maleadt merged commit cfbad5f into master Feb 14, 2017
@tkelman tkelman deleted the tb/display_error branch February 14, 2017 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants