Bug 58339 - [PATCH] Make OFFSET() accept missing parameters
Summary: [PATCH] Make OFFSET() accept missing parameters
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 3.13-dev
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2015-09-07 11:37 UTC by Patrick Zimmermann
Modified: 2016-02-17 08:53 UTC (History)
0 users



Attachments
offset_missing_arg.patch (1.30 KB, patch)
2015-09-07 11:37 UTC, Patrick Zimmermann
Details | Diff
FormulaEvalTestData.xls (174.00 KB, application/octet-stream)
2015-09-07 11:38 UTC, Patrick Zimmermann
Details
FormulaEvalTestData_Copy.xlsx (63.67 KB, application/octet-stream)
2015-09-07 11:38 UTC, Patrick Zimmermann
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Zimmermann 2015-09-07 11:37:11 UTC
Created attachment 33072 [details]
offset_missing_arg.patch

This patch makes the OFFSET function also work with one or both of the last two parameters missing. Excel 2013 also behaves that way.

I'll attach the changed test files separately.
test-data/spreadsheet/FormulaEvalTestData_Copy.xlsx
test-data/spreadsheet/FormulaEvalTestData.xls
Comment 1 Patrick Zimmermann 2015-09-07 11:38:14 UTC
Created attachment 33073 [details]
FormulaEvalTestData.xls
Comment 2 Patrick Zimmermann 2015-09-07 11:38:57 UTC
Created attachment 33074 [details]
FormulaEvalTestData_Copy.xlsx
Comment 3 Patrick Zimmermann 2016-02-15 09:29:23 UTC
Is there anything I can do to get this merged or is it just missing tuits?
Comment 4 Javen O'Neal 2016-02-15 18:50:31 UTC
Is there an ignored failing unit test to show that the offset function doesn't work when one or both of the last two parameters are missing? If not, that's probably all this bug needs before it can be merged.
Comment 5 Javen O'Neal 2016-02-15 19:17:24 UTC
Nevermind, TestFormulasFromSpreadsheet.java handles the extra tests. No need for new unit tests.

The current attached xls/xlsx files fail at TestFormulasFromSpreadsheet.java because their locale is DE, so they use OktIndex rather than Oct2Dec, and other locale-specific functions. However, Offset passes, so the changes are good.

I don't have access to EN locale Excel to copy your edits over at the moment. I'll get to this in a week if no one else beats me to it. Presumably the only changes were the addition of Offset formulas:

H1040 =SUM(OFFSET(K7:L8,0,0,1,1))
I1040 =SUM(OFFSET(K7:L8,0,0,1,))
J1040 =SUM(OFFSET(K7:L8,0,0,,1))
K1040 =SUM(OFFSET(K7:L8,0,0,1))
L1040 =SUM(OFFSET(K7:L8,0,0))
M1040 =SUM(OFFSET(K7:L8,0,0,,))
N1040 =SUM(OFFSET(K7:L8,0,0,))
Comment 6 Javen O'Neal 2016-02-15 20:30:49 UTC
Merged in r1730606 and r1730607.

Thanks for the patch, Patrick!
Comment 7 Patrick Zimmermann 2016-02-17 08:53:01 UTC
As you said, those seven added tests were the only changes in the Excel files.