apache
BugZilla 67646 - allow append rows to streaming workbooks
This patch works already in productive software. It allows to read a workbook using streaming workbook and append rows to it. The problem was the pointer to the last row was not set correctly in the SXSSFSheet. It is fixed now and the code changes are marked with the Bugzilla ticket id.
this has a similar set of changes to https://github.com/apache/poi/pull/528
we can't accept PRs that arbitrarily change unrelated code
Changing the return type of getRow() could break user code as it is a public method in a public class, so I think this can not be approved as it is now.
IMHO, we also need a junit test for this that tests the new functionality.
There are also a couple of other things to clean up, like spelling errors introduced by the PR and directly throwing RuntimeException. I think we should not throw RuntimeException in any of these cases. I don't know if the other committers agree with the exception changes that I have proposed instead.
if there is no test - then we shouldn't merge
Hi,
There was a code breaking changes in the past! This class is not really a typical class for direct public usage. On the other side we get a huge enhancement by using the streaming workbook also in a use case where the workbook is not created from scratch and can be appended.
I will write a test, this is a reasonable request.
Best regards Jan Lolling
On 26. Feb 2024, at 17:57, PJ Fanning @.***> wrote:
Changing the return type of getRow() could break user code as it is a public method in a public class, so I think this can not be approved as it is now.
IMHO, we also need a junit test for this that tests the new functionality.
There are also a couple of other things to clean up, like spelling errors introduced by the PR and directly throwing RuntimeException. I think we should not throw RuntimeException in any of these cases. I don't know if the other committers agree with the exception changes that I have proposed instead.
if there is no test - then we shouldn't merge
— Reply to this email directly, view it on GitHub https://github.com/apache/poi/pull/600#issuecomment-1964646892, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5KS7MWJVYN57H5TIPNGGDYVS5I3AVCNFSM6AAAAABD2LJQK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUGY2DMOBZGI. You are receiving this because you authored the thread.
Sorry, I need to build the lib for my own and changed the version number. That’s why I have changed the number back. It was not my intention to interfere your versioning.
On 27. Feb 2024, at 14:40, PJ Fanning @.***> wrote:
@pjfanning commented on this pull request.
In build.gradle https://github.com/apache/poi/pull/600#discussion_r1504252267:
@@ -100,7 +100,7 @@ allprojects { // apply plugin: 'eclipse' apply plugin: 'idea'
- version = '5.2.5-SNAPSHOT' why can't the PR just change code related to the title of the PR? - all other stuff must be reverted
— Reply to this email directly, view it on GitHub https://github.com/apache/poi/pull/600#pullrequestreview-1903540112, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5KS7NOLF7PFM4RGIYLPDTYVXO6DAVCNFSM6AAAAABD2LJQK2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBTGU2DAMJRGI. You are receiving this because you authored the thread.
@jlolling can we pause this PR for a few days? I'm concerned that we have no justification for this change - no use case defined by you - no test cases for us to think through.
The code today is deliberate. It is not an oversight to not support appending rows in SXSSF code base. SXSSF code is mainly for writing fresh workbooks with less memory overhead than the XSSF code. The idea that you can provide an XSSFWorkbook as a template is about allowing styles and other metadata to be provided.
Supporting appends to existing sheets gets us into a slippery slope to trying to support editing of existing sheets. Frankly, without justification for this change, I would say why not use XSSFWorkbook?
I'm not against appends as a general rule but I would like to concentrate on very specific use cases and then carefully document what is supported and what is not.
It may be possible to support a very precise append scenario that does not involve us changing the getRow API. The changes you have made in this PR look to have big impacts. We may be able to support your use case with much less change.
Frankly, you should never start by giving us a random PR with no context - especially one with so many unwanted changes to exceptions and git styles (etc.) - so much extraneous unnecessary fluff that I can't yet work out what it is you want to do.
Hi pjfanning,
yes of course. It is not my intention to gain all attention. I will write testcases and will explain better what is my intention for this change. Best regards Jan