search

mjul / docjure

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

Allow creating workbooks with multiple sheets #49

Closed dpetranek closed 8 years ago

dpetranek commented 8 years ago

Modifies the create-workbook function without breaking backwards compatibly. The original arity of [sheetname data] is preserved, but I added an arity of [sheet-vec] that takes a vector of alternating sheetnames and sheetdata.

dpetranek commented 8 years ago

Currently, not all the tests are passing - specifically checking row numbers. I think it's a problem with how I wrote the test instead of the functionality, though.

slipset commented 8 years ago

@dpetranek I'd be very interested in this functionality :)

madstap commented 8 years ago

@dpetranek Nice, I was just thinking about how the create-workbook function should have this and then I saw your PR.

The version I pictured uses variable arity though, [sheet-name data & name-data-pairs], the rationale being that sheet names and data are key value pairs and create-workbook should work like hash-map or assoc, cause that's what I understand from the original arity. Why are the pairs wrapped in a vector?

dpetranek commented 8 years ago

@madstap I like your idea of using variable arity, I think it's much more intuitive for users to have consistent syntax for this function between the single-sheet and multiple sheet versions. And your cleaned up version is much less hacky than mine was, and a distinct improvement for readability, so I'm going with that.

I also like the :pre test to weed out unmatched pairs. I initially decided to be forgiving, and just allow it to create a blank sheet because I sometimes need to do that. But I can just pass nilif I need a blank sheet, and I think it's better to check our expectations at the door.

I've committed the new version with cleaned up tests, everything is passing now.

dpetranek commented 8 years ago

@mjul Hello Martin, could you take a look at this? @madstap helped me clean up the implementation to simplify and I think it stands pretty well on its own now.

The tests are based on those for the create-workbook function and they are all passing.

I'm about to start a new project and I'd like to see if we can get this merged before then. Thanks!

dpetranek commented 8 years ago

I had commented out the pomegranate dependency for reasons that are now lost to the mists of time, but I just uncommented them and re-ran the tests, everything is still good.

mjul commented 8 years ago

Looks good. Please add your names as contributors in the Readme and you are good to go.

dpetranek commented 8 years ago

I added mine. Your turn @madstap.

madstap commented 8 years ago

Could you just add it? [Aleksander Madland Stapnes](https://github.com/madstap) (madstap) Or should I make a pull request?

dpetranek commented 8 years ago

@madstap I added you. @mjul Looks like we're good to go. Thanks!

dparis commented 8 years ago

Hello all! Any chance this could be merged in the next day or so? If not, I can just use @dpetranek's branch directly.

Thanks for all your work on this library!

mjul commented 8 years ago

Thanks for the great work!

mjul commented 8 years ago

Hi Dylan

I just merged and published as the latest snapshot build to Clojars. Have fun with it and have a nice day, and if you have some ideas for improvements, don’t hesitate to send a pull request ;-)

Cheers, Martin

mjul commented 8 years ago

This is now available in the 1.11-SNAPSHOT build on Clojars:

https://clojars.org/dk.ative/docjure/versions/1.11.0-SNAPSHOT

dparis commented 8 years ago

@mjul Thank you very much, I look forward to using it! And yes, I'll be sure to send a PR if we come up with any fixes/improvements.

@dpetranek @madstap Thanks as well for your work on contributing this feature!