Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 4, 2018

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