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

Allow ignoring of missing workbooks #57

Closed
wants to merge 1 commit into from

Conversation

tombooth
Copy link

@tombooth tombooth commented Nov 8, 2016

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.

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
Copy link
Author

tombooth commented Nov 8, 2016

This neatens, adds a test and brings up to do date #16

@mjul
Copy link
Owner

mjul commented Nov 9, 2016

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
Copy link
Author

How are you imagining it would work?

  • Replace everything that is currently a workbook/sheet/cell with a clojure map wrapping those values up?
  • Would we still have select-sheet returning updated maps with a :sheet value set or something?
  • How would you imagine failure happening when I pass a map into a function requiring a sheet but one doesn't exist?

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
Copy link
Owner

mjul commented Nov 15, 2016

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
Copy link
Owner

mjul commented Nov 15, 2016

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

@mjul mjul closed this Nov 15, 2016
@kornysietsma
Copy link
Contributor

kornysietsma commented Nov 28, 2016 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants