add xlsx support #26
add xlsx support #26
Conversation
|
|
||
| // Meta data | ||
| mc := make(chan map[string]string, 1) | ||
| go func() { |
There was a problem hiding this comment.
You don't need to use a go routine here.
|
|
||
| // Document body | ||
| bc := make(chan string, 1) | ||
| go func() { |
There was a problem hiding this comment.
Don't need a go routine here either.
| // Document body | ||
| bc := make(chan string, 1) | ||
| go func() { | ||
| var body string |
There was a problem hiding this comment.
Potentially a lot of string concat operations here, you should use a bytes.Buffer instead.
| "io" | ||
| ) | ||
|
|
||
| // Convert MS Excel Spreadsheet |
There was a problem hiding this comment.
For exported functions the comment should begin with the name of the function:
// ConvertXLSX ...This looks like it converts XLSX to something similar to CSV, but isn't (i.e. there doesn't seem to be any escaping): if a cell includes , then the output will include an extra column. Definitely clarify this in the comment.
You probably want use https://golang.org/pkg/encoding/csv/#Writer from the standard library if you need CSV.
| for _, sheet := range xlsFile.Sheets { | ||
| for rowIndex, row := range sheet.Rows { | ||
| for cellIndex, cell := range row.Cells { | ||
| text, _ := cell.String() |
There was a problem hiding this comment.
Don't ignore the error here. From the docs for the XLSX lib you're using, it will return the raw value of the cell, and hence incorrectly formatted. I would assume that the formatting string in the spreadsheet is correct, and the XLSX library is failing to parse it and so the output won't properly represent the value as shown in the spreadsheet: you don't want to fail silently.
|
|
||
| import ( | ||
| "fmt" | ||
| "github.com/tealeg/xlsx" |
There was a problem hiding this comment.
Put this import underneath the others (it's non-std lib).
| go func() { | ||
| var body string | ||
| for _, sheet := range xlsFile.Sheets { | ||
| for rowIndex, row := range sheet.Rows { |
There was a problem hiding this comment.
It's a convention to use smaller names in Go, so rowIndex -> i here instead?
| var body string | ||
| for _, sheet := range xlsFile.Sheets { | ||
| for rowIndex, row := range sheet.Rows { | ||
| for cellIndex, cell := range row.Cells { |
|
Further to what @mish15 said: We use this library internally to convert documents into (unstructured) plain text (i.e. paragraphs of text). Looking at XLSX -> CSV is very different: there is definitely structure, and it's being mostly preserved in the conversion. I can't see any situation where we would want to read CSV and assume that it is equivalent to a paragraph of text - at least not without more pre-processing first. The license terms on the library are a real problem. Including copyright information in everything that uses docconv is a hassle and not something that we want to start doing unless it's really essential to the library. |
I've added xlsx support based on this lib: https://github.com/tealeg/xlsx