mjul / docjure

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

xls wb creation, new font and cell-style options #23

Closed naipmoro closed 9 years ago

naipmoro commented 10 years ago

These changes arose from my need to create xls workbooks with an assortment of font and cell-style options.

naipmoro commented 10 years ago

Today I eliminated the setCellValue reflection warning with a multimethod redefinition of set-cell!

mjul commented 10 years ago

Thanks a lot, this is excellent work!

I will be happy to merge it in if you would help eliminate the code duplication in the XLSX and XLS tests. I don't want to end up in a situation where the test suite becomes a pain to maintain.

The best is probably to have XLS tests for only the create/load/save behaviour as most of the other code is shared an already tested in existing tests against the XLSX sheets.

Alternatively, consider parametrizing the test suite on the sheet type (this is probably more work, and with little extra benefit so the former is preferred).

naipmoro commented 10 years ago

Thanks for the encouragement.

I believe I've already removed all redundant tests from xls-test. Every test there requires either create-xls-workbook or an explicit use of HSSFWorkbook. I'm certainly not happy having an extra file. Originally, I thought parametrizing the tests to cover both xls and xlsx would be easy, but I couldn't solve the problem at the time. I did come up with a macro approach. Example test:

(defmacro read-cell-test [creator]
   `(let [sheet-data# [["Nil" "Blank" "Date" "String" "Number"]
                       [nil "" ~(july 1) "foo" 42.0]]
          wb# (~creator "Sheet 1" sheet-data#)
          sheet# (.getSheetAt wb# 0)
          rows#  (vec (iterator-seq (.iterator sheet#)))
          data-row# (vec (iterator-seq (.cellIterator (second rows#))))
          [nil-cell# blank-cell# date-cell# string-cell# number-cell#] data-row#]
      (testing "Should read all cell types"
        (is (= 2 (count rows#)))
        (is (nil? (read-cell nil-cell#)))
        (is (= "" (read-cell blank-cell#)))
        (is (= ~(july 1) (read-cell date-cell#)))
        (is (= 42.0 (read-cell number-cell#))))))

 (deftest read-cell-test-xlsx (read-cell-test create-workbook))
 (deftest read-cell-test-xls (read-cell-test create-xls-workbook))

This worked for the couple of tests I tried it on (except on clojure 1.3, which probably should be retired in any case!), but writing a macro for every test is crazy. There's probably a very simple solution I haven't thought of. I'm also playing around with using stencil templates -- I'll see how that goes. But really, despite the clunkiness, copying-and-pasting and changing create-workbook to create-xls-workbook seems the simplest way so far.

Speaking of the create-xls-workbook function, it's pretty hacky. I'd prefer having a :xls true option for create-workbook instead, where the default would be false. If you're interested in that, I could implement it.

Regards, naipmoro

naipmoro commented 10 years ago

I added the ability to set cell comments with set-cell-comment! It shows a comment-box when the cell is hovered over and takes 3 options:

:font (font | map of font options) :width (int - the width of the comment box in columns; default 1 cols) :height (int - the height of the comment box in rows; default 2 rows)

Added a test. However...it only tests the string. I don't see a way to retrieve the font from the comment box.

mjul commented 9 years ago

Thanks, I have merged it and released v1.7.0 to Clojars.