better error messages (#76)

* better error messages

Author: SimonDanisch

src/error_handling.jl

@@ -53,10 +53,24 @@ function handle_current_error(e, library, islast::Bool)
 end
 handle_current_error(e::NotInstalledError) = warn(string("lib ", e.library, " not installed, trying next library"))
 
+
 """
 Handles a list of thrown errors after no IO library was found working
 """
-function handle_error(exceptions::Vector)
+function handle_exceptions(exceptions::Vector, action)
+    # first show all errors when there are more then one
+    multiple = length(exceptions) > 1
+    println("Error$(multiple?"s" : "") encountered while $action.")
+    if multiple
+        println("All errors:")
+        for (err, file) in exceptions
+            println("   ", err)
+        end
+    end
+    # then handle all errors.
+    # this way first fatal exception throws and user can still see all errors
+    # TODO, don't throw, if it contains a NotInstalledError?!
+    println("Fatal error:")
     for exception in exceptions
         continue_ = handle_error(exception...)
         continue_ || break
@@ -66,10 +80,10 @@ end
 handle_error(e, q) = rethrow(e)
 
 function handle_error(e::NotInstalledError, q)
-    println("Library ", e.library, " is not installed but can load format: ", q)
+    println("Library \"", e.library, "\" is not installed but is recommended as a library to load format: \"", file_extension(q), "\"")
     !isinteractive() && rethrow(e) # if we're not in interactive mode just throw
     while true
-        println("should we install ", e.library, " for you? (y/n):")
+        println("Should we install \"", e.library, "\" for you? (y/n):")
         input = lowercase(chomp(strip(readline(STDIN))))
         if input == "y"
             info(string("Start installing ", e.library, "..."))

src/loadsave.jl

@@ -74,8 +74,8 @@ function load{F}(q::Formatted{F}, args...; options...)
         isfile(filename(q)) || open(filename(q))  # force systemerror
         throw(UnknownFormat(q))
     end
-    libraries   = applicable_loaders(q)
-    failures    = Any[]
+    libraries = applicable_loaders(q)
+    failures  = Any[]
     for library in libraries
         try
             Library = checked_import(library)
@@ -84,16 +84,15 @@ function load{F}(q::Formatted{F}, args...; options...)
             end
             return Library.load(q, args...; options...)
         catch e
-            handle_current_error(e, library, library == last(libraries))
             push!(failures, (e, q))
         end
     end
-    handle_error(failures)
+    handle_exceptions(failures, "loading \"$(filename(q))\"")
 end
 function save{F}(q::Formatted{F}, data...; options...)
     unknown(q) && throw(UnknownFormat(q))
-    libraries   = applicable_savers(q)
-    failures    = Any[]
+    libraries = applicable_savers(q)
+    failures  = Any[]
     for library in libraries
         try
             Library = checked_import(library)
@@ -102,9 +101,8 @@ function save{F}(q::Formatted{F}, data...; options...)
             end
             return Library.save(q, data...; options...)
         catch e
-            handle_current_error(e, library, library == last(libraries))
             push!(failures, (e, q))
         end
     end
-    handle_error(failures)
+    handle_exceptions(failures, "saving \"$(filename(q))\"")
 end

src/query.jl

@@ -46,7 +46,8 @@ end
 const unknown_df = DataFormat{:UNKNOWN}
 
 """
-`unknown(f)` returns true if the format of `f` is unknown."""
+`unknown(f)` returns true if the format of `f` is unknown.
+"""
 unknown(::Type{format"UNKNOWN"})    = true
 unknown{sym}(::Type{DataFormat{sym}}) = false
 
@@ -155,7 +156,8 @@ end
 
 """
 `info(fmt)` returns the magic bytes/extension information for
-`DataFormat` `fmt`."""
+`DataFormat` `fmt`.
+"""
 Base.info{sym}(::Type{DataFormat{sym}}) = sym2info[sym]
 
 
@@ -217,7 +219,8 @@ abstract Formatted{F<:DataFormat}   # A specific file or stream
 """
 `File(fmt, filename)` indicates that `filename` is a file of known
 DataFormat `fmt`.  For example, `File{fmtpng}(filename)` would indicate a PNG
-file."""
+file.
+"""
 immutable File{F<:DataFormat} <: Formatted{F}
     filename::Compat.UTF8String
 end
@@ -239,7 +242,8 @@ file_extension(f::File) = splitext(filename(f))[2]
 `Stream(fmt, io, [filename])` indicates that the stream `io` is
 written in known `Format`.  For example, `Stream{PNG}(io)` would
 indicate PNG format.  If known, the optional `filename` argument can
-be used to improve error messages, etc."""
+be used to improve error messages, etc.
+"""
 immutable Stream{F<:DataFormat,IOtype<:IO} <: Formatted{F}
     io::IOtype
     filename::Nullable{Compat.UTF8String}
@@ -255,7 +259,8 @@ stream(s::Stream) = s.io
 
 """
 `filename(stream)` returns a nullable-string of the filename
-associated with `Stream` `stream`."""
+associated with `Stream` `stream`.
+"""
 filename(s::Stream) = s.filename
 
 """

test/error_handling.jl

@@ -1,29 +1,30 @@
+println("these tests will print warnings: ")
 context("Not installed") do
-	eval(Base, :(is_interactive = true)) # for interactive error handling
+    eval(Base, :(is_interactive = true)) # for interactive error handling
 
-	add_format(format"NotInstalled", (), ".not_installed", [:NotInstalled])
-	stdin_copy = STDIN
+    add_format(format"NotInstalled", (), ".not_installed", [:NotInstalled])
+    stdin_copy = STDIN
     stderr_copy = STDERR
     rs, wr = redirect_stdin()
-	rserr, wrerr = redirect_stderr()
-	ref = @async save("test.not_installed")
-	println(wr, "y")
-	if VERSION < v"0.4.0-dev"
-		@fact_throws ErrorException wait(ref) #("unknown package NotInstalled")
-	else
-		@fact_throws CompositeException wait(ref) #("unknown package NotInstalled")
-	end
-	ref = @async save("test.not_installed")
-	println(wr, "invalid") #test invalid input
-	println(wr, "n") # don't install
-	wait(ref)
-	@fact istaskdone(ref) --> true
+    rserr, wrerr = redirect_stderr()
+    ref = @async save("test.not_installed")
+    println(wr, "y")
+    if VERSION < v"0.4.0-dev"
+        @fact_throws ErrorException wait(ref) #("unknown package NotInstalled")
+    else
+        @fact_throws CompositeException wait(ref) #("unknown package NotInstalled")
+    end
+    ref = @async save("test.not_installed")
+    println(wr, "invalid") #test invalid input
+    println(wr, "n") # don't install
+    wait(ref)
+    @fact istaskdone(ref) --> true
 
     close(rs);close(wr);close(rserr);close(wrerr)
-	redirect_stdin(stdin_copy)
+    redirect_stdin(stdin_copy)
     redirect_stderr(stderr_copy)
 
-	eval(Base, :(is_interactive = false)) # for interactive error handling
+    eval(Base, :(is_interactive = false)) # for interactive error handling
 
 end
 
@@ -41,3 +42,24 @@ context("Absent implementation") do
     @fact_throws FileIO.WriterError save(Stream(format"BROKEN", STDOUT))
     redirect_stderr(stderr_copy)
 end
+
+module MultiError1
+import FileIO: @format_str, File
+load(file::File{format"MultiError"}) = error("1")
+end
+module MultiError2
+import FileIO: @format_str, File, magic
+load(file::File{format"MultiError"}) = error("2")
+end
+context("multiple errors") do
+    println("this test will print warnings: ")
+    add_format(
+        format"MultiError",
+        (),
+        ".multierr",
+        [:MultiError1],
+        [:MultiError2]
+    )
+    ret = @test_throws ErrorException load("test.multierr")
+    #@test ret.value.msg == "1" # this is 0.5 only
+end

test/query.jl

@@ -54,6 +54,9 @@ end
 
 end
 
+
+
+
 try
     empty!(FileIO.ext2sym)
     empty!(FileIO.magic_list)
@@ -235,8 +238,6 @@ try
             [:LoadTest1, FileIO.LOAD, OSKey],
             [:LoadTest2]
         )
-        stderr_copy = STDERR
-        rd,wr = redirect_stderr()
         @fact lensave0 + 1 --> length(FileIO.sym2saver)
         @fact lenload0 + 1 --> length(FileIO.sym2loader)
         @fact length(FileIO.sym2loader[:MultiLib]) --> 2
@@ -252,9 +253,8 @@ try
         @fact isdefined(:LoadTest1) --> true # first module should load first but fail
         @fact x --> 42
         rm(fn)
-        close(rd);close(wr)
-        redirect_stderr(stderr_copy)
     end
+
 finally
     # Restore the registry
     empty!(FileIO.ext2sym)

test/runtests.jl

@@ -1,5 +1,6 @@
 using FileIO
 using FactCheck
+using Base.Test
 facts("FileIO") do
 	include("query.jl")
 	include("loadsave.jl")