feat(xlsx): initial support for xlsx data format, identity dsio #170
Conversation
b5
added
feat
ghost
added
the
in a world where we have more time I'd break these commits up. This adds initial support for the xlsx data format as readers/writers. Also added: initial support for "identity" dsio read/write for data that's already been parsed. We're going to need this later on for working with inlined dataset bodies. currently not in use, and shouldn't be used until we have tests. also fixed an issue with dsutil.FormFileDataset that wasn't properly assigning form values to the returned dataset.
data_format.go
Outdated
| @@ -30,9 +30,9 @@ const ( | |||
| // XMLDataFormat specifies eXtensible Markup Language-formatted data | |||
| // currently not supported. | |||
| XMLDataFormat | |||
| // XLSDataFormat specifies microsoft excel formatted data | |||
| // XLSXDataFormat specifies microsoft excel formatted data | |||
| // currently not supported. | |||
There was a problem hiding this comment.
Remove "currently not supported". I understand this is only partial support, but I think it's enough that it moves beyond "not supported".
|
|
||
| // Map structures XLSXOptions as a map of string keys to values | ||
| func (o *XLSXOptions) Map() map[string]interface{} { | ||
| if o == nil { |
There was a problem hiding this comment.
Do we need to check nil here? In the rest of codebase, we're assuming that interface types won't contains a nil pointer value, for example see lib/datasets.go. Seems like an anti-pattern to support it here, and the code will just crash a few lines below where we try to resolve o.SheetName.
There was a problem hiding this comment.
Totally agreed with this. All three FormatConfig variants have this quirk, so I'd like to remove it in a separate PR with some proper checks. I'll file an issue
dsio/xslx.go
Outdated
| rdr.r, rdr.err = rdr.file.Rows(rdr.sheetName) | ||
| } | ||
|
|
||
| return rdr, nil |
There was a problem hiding this comment.
I'm unsure, but should this be return rdr, rdr.err? Otherwise the error from Rows is being ignored.
dsio/xslx.go
Outdated
|
|
||
| // Get the largestDenominator that is a multiple of a basedDenominator | ||
| // and fits at least once into a given numerator. | ||
| func getLargestDenominator(numerator, multiple, baseDenominator, power int) (int, int) { |
There was a problem hiding this comment.
I realize this function is coming from another source, but I find it pretty confusing, and a bit over complicated. How about:
// largestLesserExponent returns the largest exponent of base that does not exceed num
// equivalent to pow(floor(log(num, base)), base)
func largestLesserExponent(num, base int) int {
prev := 1
exp := base
for num >= exp {
prev = exp
exp = exp * base
}
return prev
}
in a world where we have more time I'd break these commits up. This adds initial support for the xlsx data format as readers/writers.
Also added: initial support for "identity" dsio read/write for data that's already been parsed. We're going to need this later on for working with inlined dataset bodies. currently not in use, and shouldn't be used until we have tests.
The one change this does require is adding a
Closemethod on dsio.EntryReader, which to me should probably be there anyway.also fixed an issue with dsutil.FormFileDataset that wasn't properly assigning form values to the returned dataset.