Headers errors kill the process

[weavermedia]

Attempting to import a spreadsheet with incorrect headers is detected in the gem's load_header method but it does a raise error which stops the import process dead unless I wrap the import call in a rescue block with:

begin
  UserImporter.import(file.path, extension: :csv)
rescue => e
  puts e.message
end

This makes every call to import quite verbose and unnecessary when there is already an elegant :import_failed handler.

Maybe it would be better to fire the :import_failed event when load_header fails? This would allow me to handle the error in the same way as I handle other errors like row validations.

[gnapse]

This was how the library behaved initially, but I received the opposite complaint, and frankly I think that keeping a fatal error silent, only to be handled by the :import_failed, is not good, since in most occasions, the context where the importer is invoked, should be aware of its failure.

The event only serves the purpose of letting the importer handle its own failures, but it does not let the caller know about the error. Unlike the row-level errors, which are understandably minor ones, and should not make the whole process fail, a fatal error, such as the file not being readable, should not be swallowed by the importer.

Is it really a big deal to have to wrap a call to an importer in a begin/rescue block? Perhaps we could pass an option parameter to specify that fatal errors should not be raised.

[weavermedia]

OK, I see your point but I don't entirely agree :-)

I agree that the context where import is invoked should be aware of a failure and that's where I see a success/failure message being passed back.

I don't feel that incorrect headers qualify as a 'fatal' error. The file loads and is read successfully, it's just that the import process can't go anywhere because the user supplied the wrong column headers. I don't see that as a reason to throw an app-wide fatal error.

On the other hand the code will quietly pass back a invalid byte sequence in UTF-8 error if the file literally isn't readable.

I agree that, no, it isn't a big deal to wrap the call but if you keep the current setup then maybe you could more clearly document that incorrect column headers will cause a fatal error and stop the app, and that all calls to import should be wrapped with a rescue.

[weavermedia]

Sorry, I closed it accidentally. Such are the joys of keyboard shortcuts.

[gnapse]

So instead of an exception, you propose to give back the info to the caller via a return value, isn't it? What do you suggest this return value should be, so that it transmit to the caller the success or failure of the import process? It certainly cannot be boolean, because the failure could be due to a number of reasons (the exception type currently conveys that information). Also in case of not failure, it could also be useful to let the caller know some information about the number of rows that were imported, etc.

[weavermedia]

I propose that we are able to build that return string value ourselves using the row_* events and pass it back via the import_finished event.

See the row events in my DataImporter base class in #19 for an example of how I'm building this message.