Skip to content

Comments

Fixed handling of string based formulas. (#153)#154

Merged
monitorjbl merged 1 commit intomonitorjbl:masterfrom
zwom:fixWrongTypeForStringFormula
Oct 4, 2018
Merged

Fixed handling of string based formulas. (#153)#154
monitorjbl merged 1 commit intomonitorjbl:masterfrom
zwom:fixWrongTypeForStringFormula

Conversation

@zwom
Copy link

@zwom zwom commented Jul 25, 2018

The celltype str used to be handled as if it has the celltype CellType.FORMULA. Even if it only occurs as a result of a formula, it should still be handled as a string.
This bug is fixed by using a boolean flag in the StreamingCell.java, that is true when the element is found in the excel file.
Using the method getCellTypeEnum() now returns CellType.STRING for str as well. It now also checks if the the cell has the formulaType and will return CellType.FORMULA if the boolean flag is true.
The method getCachedFormulaResultTypeEnum() will only return a cellType if the boolean flag is true and will also return CellType.String for str.

Included is a unittest covering the bug.

Let me know if you need any more information, of if I should change something in the code.

Fixed getting the wrong celltype for cached celltypes, when result is a
string, by saving the formula type as a boolean flag in the
StreamingCell and handling formula strings("t=str") as a normal string.
@monitorjbl
Copy link
Owner

Sorry it's taken me so long to review this, I had kind of a crazy summer and haven't had any time for my GitHub projects (or much of anything else, really). Would it be alright if this change waited until the update to POI 4.0.0 goes through? It looks like @pjfanning already has applied this to his fork post-upgrade, so merging it now would cause a few merge conflicts.

@pjfanning
Copy link
Contributor

@monitorjbl this is not mixed in with my pull request to your project - I have a fork of my own that I'm maintaining independently (for now) - so it's safe for you to merge this

@monitorjbl monitorjbl mentioned this pull request Oct 4, 2018
@monitorjbl monitorjbl merged commit c0fa347 into monitorjbl:master Oct 4, 2018
@chkal
Copy link
Contributor

chkal commented Oct 5, 2018

Thanks a lot for merging. This was a major blocker for us!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants