mjul / docjure

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

Allow ignoring of missing workbooks #57

Closed tombooth closed 7 years ago

tombooth commented 7 years ago

During the evaluation of a formula, if POI cannot resolve a referenced spreadsheet it will throw:

CollaboratingWorkbooksEnvironment.WorkbookNotFoundException

Sometimes this is not the desired behaviour and POI provides a way of telling the evaluator to ignore these failures and just returned the cached value for the cell using setIgnoreMissingWorkbooks.

In order to expose this behaviour we have used a dynamic var so that we can wrap the processing of a whole spreadsheet in a binding, rather than passing properties through to each call of read-cell.

tombooth commented 7 years ago

This neatens, adds a test and brings up to do date https://github.com/mjul/docjure/pull/16

mjul commented 7 years ago

Hi Tom,

thanks for submitting this, it is a really nice feature.

In the long term, I think we would be better off to move towards lifting the configuration from the dynamic var to into an explicit Clojure state object that would be the input to all the Docjure functions, rather than passing around the underlying POI object which is the case now. (The POI object would then be hidden inside this new structure).

It would also allow us to clean up the imperative code for the workbook styling features.

Is this something you would like to help with?

Cheers, Martin

tombooth commented 7 years ago

How are you imagining it would work?

If I can get a solid understanding of how you think this should work I might be able to work on it. As you can imagine the reason for the PR is it is blocking a story I am currently working on. We've had to release our own version of this lib before when the previous version of the PR didn't get merged and I'd like to avoid doing that again.

mjul commented 7 years ago

I had something like that in mind.

You made a good point about select-sheet.

My first thought is that all the functions should work in a higher world represented by operating on the Clojure map you describe, and there should be a function to pull the data out of the monad into the lower-order "normal" world returning something that is no longer composable (akin to the select-sheet returning not a POI object but the data).

We could still use the existing functions operating on POI instances as private implementation but we would need to provide new public API with the Clojure maps. Then we would unwrap the POI value, use the existing function and re-wrap it into the Clojure map (you may notice this sounds like monadic bind).

Any data-returning functions could even just add the last data to the map, e.g. :result or something and we could add a get-result function as the only way out of the monad. This would explicitly delineate the higher-order, composable world, and the outside world of working with the data as plain Clojure data structures.

If we define the public API right we might be able to provide extensible, configurable behaviours like the one you need without opening up the library itself, your case of missing workbooks being one example. Please give some thought to how we could accomplish this (maybe something with multi-methods where the library has only the default and client code can add more cases?)

When the Clojure-map-instead-of-POI-instances is in place, we can create a much nicer API for the sheet styling, as the Clojure state map could provide info on the styles created etc. so that it would not be necessary to add the styles imperatively as is the case today.

I am going to create a v2-branch for this work as it will not be 100% backwards compatible.

Maybe other contributors would like to chip in?

cc: @cbaatz @mva @ragnard @vijaykiran @jonneale @naipmoro @nidu @oholworthy @rakhra @igortn @reisub @bendisposto @stuarth @dpetranek @madstap @kornysietsma @lokori @alephyud

mjul commented 7 years ago

I have created an ticket for doing this here: Issue #58

kornysietsma commented 7 years ago

Hey - this sounds like a great idea, and I'd like to help out... but my wife is expecting our first baby any day now, so my ability to contribute might be minimal right now :)

On 15 November 2016 at 09:56, Martin Jul notifications@github.com wrote:

I had something like that in mind.

You made a good point about select-sheet.

My first thought is that all the functions should work in a higher world represented by operating on the Clojure map you describe, and there should be a function to pull the data out of the monad into the lower-order "normal" world returning something that is no longer composable (akin to the select-sheet returning not a POI object but the data).

We could still use the existing functions operating on POI instances as private implementation but we would need to provide new public API with the Clojure maps. Then we would unwrap the POI value, use the existing function and re-wrap it into the Clojure map (you may notice this sounds like monadic bind).

Any data-returning functions could even just add the last data to the map, e.g. :result or something and we could add a get-result function as the only way out of the monad. This would explicitly delineate the higher-order, composable world, and the outside world of working with the data as plain Clojure data structures.

If we define the public API right we might be able to provide extensible, configurable behaviours like the one you need without opening up the library itself, your case of missing workbooks being one example. Please give some thought to how we could accomplish this (maybe something with multi-methods where the library has only the default and client code can add more cases?)

When the Clojure-map-instead-of-POI-instances is in place, we can create a much nicer API for the sheet styling, as the Clojure state map could provide info on the styles created etc. so that it would not be necessary to add the styles imperatively as is the case today.

I am going to create a v2-branch for this work as it will not be 100% backwards compatible.

Maybe other contributors would like to chip in?

cc: @cbaatz https://github.com/cbaatz @mva https://github.com/mva @ragnard https://github.com/ragnard @vijaykiran https://github.com/vijaykiran @jonneale https://github.com/jonneale @naipmoro https://github.com/naipmoro @nidu https://github.com/nidu @oholworthy https://github.com/oholworthy @rakhra https://github.com/rakhra @igortn https://github.com/igortn @reisub https://github.com/reisub @bendisposto https://github.com/bendisposto @stuarth https://github.com/stuarth @dpetranek https://github.com/dpetranek @madstap https://github.com/madstap @kornysietsma https://github.com/kornysietsma @lokori https://github.com/lokori @alephyud https://github.com/alephyud

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mjul/docjure/pull/57#issuecomment-260597216, or mute the thread https://github.com/notifications/unsubscribe-auth/AABFZYr8d3mx11cLCZM-6q2kwB-2C8NHks5q-YHCgaJpZM4Ksdpr .

-- Kornelis Sietsma korny at my surname dot com http://korny.info .fnord { display: none !important; }