search

mjul / docjure

Read and write Office documents from Clojure
MIT License
622 stars 129 forks source link

Added load-workbook-as-resource. #35

Closed igortn closed 9 years ago

igortn commented 9 years ago

The existing load-workbook function has a file name as a parameter. In the case when the workbook is attempted to be loaded from a file on a classpath, which is a typical case for application servers, the server does not have access to the file system, and the FileNotFound exception is thrown.

The new function uses a named resource, instead of a file name, to load the workbook.

mjul commented 9 years ago

Thanks for the work, I would be happy to accept it if you could please provide a unit test case for it as well. Also, for extra points, consider extracting another variant of load-workbook that loads from a stream and implementing the other load methods on top of that.

igortn commented 9 years ago

Makes perfect sense. I will do that and resubmit.

Thank you, Igor

On Mon, Aug 17, 2015 at 6:48 PM, Martin Jul notifications@github.com wrote:

Thanks for the work, I would be happy to accept it if you could please provide a unit test case for it as well. Also, for extra points, consider extracting another variant of load-workbook that loads from a stream and implementing the other load methods on top of that.

— Reply to this email directly or view it on GitHub https://github.com/mjul/docjure/pull/35#issuecomment-131985552.

igortn commented 9 years ago

The clean way would be implementing 'load-workbook-as-stream' and deriving both 'load-workbook-as-file' and 'load-workbook-as-resource' on the top of it. Unfortunately, this breaks API backwards compatibility by renaming the currently existing function 'load-workbook'. Do you mind doing that, or you prefer not to change the existing function name?

Please let me know.

On Mon, Aug 17, 2015 at 10:51 PM, Igor Tovstopyat-Nelip igor.tn@gmail.com wrote:

Makes perfect sense. I will do that and resubmit.

Thank you, Igor

On Mon, Aug 17, 2015 at 6:48 PM, Martin Jul notifications@github.com wrote:

Thanks for the work, I would be happy to accept it if you could please provide a unit test case for it as well. Also, for extra points, consider extracting another variant of load-workbook that loads from a stream and implementing the other load methods on top of that.

— Reply to this email directly or view it on GitHub https://github.com/mjul/docjure/pull/35#issuecomment-131985552.

igortn commented 9 years ago

I have pushed another commit with the following:

  1. Added a dependency for a profile :test.
  2. Provided three functions: load-workbook-as-stream, load-workbook-as-file, and load-workbook-as-resource. Deprecated the function load-workbook as of version 1.9.
  3. Covered with unit tests.

Please let me know if any adjustments are needed.