Conversation
…g-reader into poi-4 # Conflicts: # src/main/java/com/monitorjbl/xlsx/sst/BufferedStringsTable.java # src/main/java/com/monitorjbl/xlsx/sst/CTRstImpl.java
|
I have a version of this change published to sonatype snaphots repo - https://github.com/pjfanning/excel-streaming-reader |
|
Hi @monitorjbl, did you already have a look at this PR? |
|
Hi @monitorjbl, |
|
@borjapenman I have a POI 4.0.0 compatible release done at https://github.com/pjfanning/excel-streaming-reader - I've just added openjdk11 testing) |
|
Hey guys, sorry for taking so long to review things. It's been kind of a hectic summer and I haven't had much time to spend on anything in GitHub. Things are (hopefully) winding down now, I'm reviewing the open PRs now. |
monitorjbl
left a comment
monitorjbl
left a comment
There was a problem hiding this comment.
There are a couple small things in this PR I'd change, but I'm good to merge this as-is if you don't have time to/don't want to address them this week so I'll go ahead and approve it now. We'll have to do a major version bump for sure, dropping JDK7 support is kind of a big deal for consumers (but a change I'm more than happy to make).
Sorry again for taking so long to review these PRs, I know it's been inconvenient for you and you've contributed quite a bit to this library.
pom.xml
Outdated
|
|
||
|
|
||
| <repositories> | ||
| <repository> |
There was a problem hiding this comment.
It looks like POI 4.0.0 is available in Maven Central now, is this still necessary?
There was a problem hiding this comment.
no this can go, now
| } finally { | ||
| if(tmp != null) { | ||
| log.debug("Deleting tmp file [" + tmp.getAbsolutePath() + "]"); | ||
| if (log.isDebugEnabled()) { |
There was a problem hiding this comment.
This seems a tad unnecessary, I'm pretty sure the logging framework will do this with just a call to log.debug()
There was a problem hiding this comment.
avoids the overhead of doing the string concat if debug is not enabled but not a big deal either way
There was a problem hiding this comment.
That's a good reason to leave it as-is. I honestly just need to do a pass through the code at some point to use the formatted string form of the logger methods. This will effectively skip string concatenation if debugging is not enabled:
log.debug("Deleting tmp file [{}]", tmp.getAbsolutePath())
I think I just didn't know about that when I initially wrote this code :(
|
When is this going to be a new release so that it can be imported in the pom? |
|
Version 2.0.0 has been published and should be available in the next few hours |
|
Thanks a lot! |
4 participants
POI 4.0.0 is about to be released. I can fix up the build file when the POI jars are fully released.