Conversation
The jobs of the old Xls[x]WorkSheet class are going to be split up between class template Sheet<T> and Xls[x]SheetData.
Another preparatory step towards de-duplicating code that operates on the cells For now, at least, just give xlsWorkbook a dummy stringTable. Add the workbook's stringTable as a parameter of inferType(), asCharSxp(), asStdString() for xls, for parallelism with xlsx. Make stringTable the last argument of those functions. The vague motivation for this is that it feels favorable for perhaps eventually making stringTable an optional argument.
This better reflects the data managed by this class. Also nice because "SheetData" comes up in the XML we work with on the xlsx side. So removes chance of confusion there.
| int ul_row = funny_min(nominal.minRow(), actual.minRow()); | ||
| int ul_col = funny_min(nominal.minCol(), actual.minCol()); | ||
| T ul_shim(std::make_pair(ul_row, ul_col)); | ||
| cells.insert(cells.begin(), ul_shim); |
There was a problem hiding this comment.
Abstract performance concern: presumably this insertion causes the entire cells vector to be copied, which would seem undesirable. That being said, I don't think this happens very often and has never been raised as a concrete performance concern.
This code is not new in this PR. It is newly located in a templated function as opposed to a duplicated member function.
| typename T::CellSet cs_; | ||
|
|
||
| public: | ||
| Sheet(const std::string& path, |
There was a problem hiding this comment.
The responsibilities of the previous Xls[x]WorkSheet class have been divided across the new Sheet template class and new Xls[x]CellSet class:
I'm not very satisfied with the Sheet name for this class.
Xls[x]CellSet: A slimmed down version of the previousXls[x]WorkSheet. This class gains access to a specific worksheet, inside a specific workbook, and slurps the cells out of it, yielding a "bunch of cells", of either the xls or xlsx variety. Basically holds all of the workbook/worksheet logic that is specific to xls vs xlsx.Sheet: Coordinates the workbook andXls[x]CellSetand uses the "bunch of cells" to create a list to return to R (where it is then made into a tibble). Given a "bunch of cells", main functions:colNames(): Read the first row’s worth of cells into a character vector of column names.colTypes(): Read the firstguess_maxrow's worth of cells and determine the most suitable type for each (not-yet-specified) column.readCols(): Read an entire “bunch of cells” into R vectors of specific types with specific names.
There was a problem hiding this comment.
Renamed to Sheet to SheetView.
| const bool trimWs, | ||
| const std::set<int>& dateFormats) { | ||
| const std::set<int>& dateFormats, | ||
| const std::vector<std::string>& stringTable) { |
There was a problem hiding this comment.
Even though the shared string table is completely abstracted away by libxls, I added a stub for it to XlsWorkBook and it newly appears in the signature of the XlsCell functions inferType(), asStdString(), and asCharSxp(). This was all in the name of removing one slight difference in the public interface of xls and xlsx, thus clearing the way for the creation of the Sheet<T> template class.
|
|
||
| XlsWorkBook(const std::string& path) { | ||
| XlsWorkBook(const std::string& path): | ||
| stringTable_{"placeholder"} |
There was a problem hiding this comment.
Recall what I said earlier about XlsWorkBook gaining a stub for a shared string table.
| const std::set<int>& dateFormats, | ||
| const std::vector<std::string>& stringTable) { |
There was a problem hiding this comment.
I changed the order of these due to vague hopes that the stringTable argument could eventually be made optional, given that it's needed for xlsx but not xls.
2 participants
Closes #284