improving format of exported excel #1456
Conversation
| @@ -35,7 +35,7 @@ | |||
|
|
|||
| @Override | |||
| public String get(String name) { | |||
| return myRecord.get(name); | |||
| return (isSet(name)) ? myRecord.get(name) : new String(); | |||
There was a problem hiding this comment.
just return "" instead of new String()
| Sheet sheet = myBook.getSheetAt(i); | ||
| if (i < myBook.getNumberOfSheets() - 1) { | ||
| int lastRow = sheet.getPhysicalNumberOfRows(); | ||
| sheet.createRow(lastRow + 1); |
There was a problem hiding this comment.
This looks a little bit weird: why do we create rows when reading? Even if it makes no changes on the disk (I believe it doesn't) it worth adding a comment about the purpose of these two rows.
There was a problem hiding this comment.
That is because of the general CSV-Reader which expects to have two empty lines between tables e.g. tasks and ressources.
| iterator = (iterator == null) ? it : Iterators.concat(iterator, it); | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
It seems that you can safely remove else block and if condition and just do what is written in the true-case block no matter if you have just 1 sheet or more than 1.
| } | ||
|
|
||
| private List<String> getCellValues(Row row) { | ||
| return Lists.newArrayList(Iterables.transform(row, Cell::getStringCellValue)); | ||
| return Lists.newArrayList(Iterables.transform(row, | ||
| (input) -> { |
There was a problem hiding this comment.
Looks like too much indent here. We use 4 spaces indent for continuations, but here the following formatting might be even better:
return Lists.newArrayList(Iterables.transform(row, (input) -> {
switch
}));
| return myDateFormat.format(input.getDateCellValue()); | ||
| } | ||
| else { | ||
| if (input.getCellStyle().getDataFormat() == (short)1) { |
There was a problem hiding this comment.
This looks as if it produces the same result as subsequent else, no? Can we just call String.valueOf(input.getNumericCellValue()) no matter what data format is? Or even just call getStringCellValue in all cases when isCellDateFormatted returns false?
There was a problem hiding this comment.
The first line is for DATE formats the second line is for INTEGER values. The parser does not like to get a String with a . when it expects an integer.
If the cell is marked as NUMERIC using getStringCellValue failes, hence the switch statements.
| @@ -44,7 +44,7 @@ public String get(String name) { | |||
| if (index == null) { | |||
| throw new IllegalArgumentException(String.format("Mapping for %s not found, expected one of %s", name, myMapping.keySet())); | |||
| } | |||
| return myValues.get(index); | |||
| return (myValues.size() <= index) ? new String() : myValues.get(index); | |||
| } | ||
|
|
||
| @Override | ||
| public void print(String value) throws IOException { | ||
| if (myCurrentRow == null) { | ||
| createNewRow(); | ||
| if (value != null) { |
There was a problem hiding this comment.
is it possible that value is nullable? What does it mean from exporter's point of view? If it is empty cell then we should probably add a cell anyway, just don't fill it with value.
There was a problem hiding this comment.
Ia m not sure if there can by empty values. I think custom fields might be null. But you are right. In these case there should be an cell regardless.
There was a problem hiding this comment.
So I looked at GanttCSVExport and it seems that there may be null values for custom fields. However, CSV printer from the CSV library replaces them with either null string according to the CSV format or with empty string. I believe XLS should also add a cell if value is null.
In fact, currently it produces wrong result. Scenario: create a single task, add two String custom columns and set the value of the second one in a task. After export the value appears in a column headed by the first custom column.
I recommend writing a test for this. In fact there are tests which export custom columns with null values to CSV in GPCsvExportTest. Perhaps it is relatively easy to piggyback on them and test with XLS as well.
|
Finally managed to look at this. Sorry it took so long. |
|
|
||
| @Override | ||
| public void print(Calendar value) throws IOException { | ||
| myCsvPrinter.print(value.toString()); |
There was a problem hiding this comment.
I believe this can also be replaced with String.valueOf like for numeric types, just for consistency.
| } | ||
|
|
||
| @Override | ||
| public void print(String value) throws IOException { | ||
| if (myCurrentRow == null) { | ||
| createNewRow(); | ||
| if (value != null) { |
There was a problem hiding this comment.
So I looked at GanttCSVExport and it seems that there may be null values for custom fields. However, CSV printer from the CSV library replaces them with either null string according to the CSV format or with empty string. I believe XLS should also add a cell if value is null.
In fact, currently it produces wrong result. Scenario: create a single task, add two String custom columns and set the value of the second one in a task. After export the value appears in a column headed by the first custom column.
I recommend writing a test for this. In fact there are tests which export custom columns with null values to CSV in GPCsvExportTest. Perhaps it is relatively easy to piggyback on them and test with XLS as well.
| Cell cell = myCurrentRow.createCell(myNextCellInd++); | ||
| if (value != null) { | ||
| cell.setCellValue(value); | ||
| private Cell addCell() throws IOException { |
There was a problem hiding this comment.
my IntelliJ tells me that IOException is never thrown from here and from a few other methods below
The exported XLS file uses strings for every exported value and a single sheet for task an resources. In order to use the file for doing some calculations you have to change the columns by hand to have the correct type and format.
I moved the formatting of exported values to the SpreadsheetWriter by overloading the print method. I also extended the interface to be able to use more than one sheet. Now the created XLS uses date and number formats for the exported values where appropriate and separate sheets for tasks and resources. The re-import of an export to XLS with new and old format is still possible. CSV im-/export remains unchanged.
This also fixes a bug where empty (custom) properties at the end of a line couldn't be read as the mapping of headers to column number was missing.
PS: When I re-imported an export of the example project the cost and coordinator properties where imported as custom properties in addition to their actual target.