[GitHub] [tika] PeterAlfredLee opened a new pull request #369: Use IOException instead of IOExceptionWithCause

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee opened a new pull request #369: Use IOException instead of IOExceptionWithCause

GitBox

PeterAlfredLee opened a new pull request #369:
URL: https://github.com/apache/tika/pull/369


   We used to use` IOExceptionWithCause` because `IOException` with the `Throwable` constructors is missing before `JDK 6`.
   I think we should use `IOException` after `JDK 6`.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox

PeterAlfredLee commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-712531011


   @kkrugler Thank you for reviewing this.
   >I don't see the IOExceptionWithCause class being deprecated
   
   `IOExceptionWithCause` in `Tika` was copying from `commons-io 1.4`.
   And `IOExceptionWithCause` in `commons-io` has be deprecated since `commons-io 2.5`. see [this](https://github.com/apache/commons-io/blob/aafc3272b38fa51d558f190fbbdac50d62027fdb/src/main/java/org/apache/commons/io/IOExceptionWithCause.java)
   
   > as removing it would be a breaking change
   
   Agree, This will break binary compatibility. And we should only break BC in major version.
   As I see, `main` branch is prepare for `Tika-2.0` which will be the new major version. So I believe this is a good time to break BC.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

kkrugler commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-712571231


   If this change is only going into main, then my question is whether you removed the `IOExceptionWithCause` class from Tika (maybe I missed that), and if not, why not?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

PeterAlfredLee commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-712624719


   I'm also agree to remove the `IOExceptionWithCause` class from Tika.
   The reason why this PR wasn't removing `IOExceptionWithCause` class is because consider [TaggedIOException](https://github.com/apache/tika/blob/main/tika-core/src/main/java/org/apache/tika/io/TaggedIOException.java) class in Tika need  `IOExceptionWithCause` class.
   
   `TaggedIOException` class in Tika was copying from `commons-io 1.4`. So I perfer not to modify `TaggedIOException` class.
   
   By the way,I think we can remove all these class from Tika which was copying from `commons-io 1.4` if we make commons-io as dependency of tika-core.
   
   WDYT ?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

kkrugler commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-712868481


   `TaggedIOException` can just extend `IOException`, yes?
   
   As to adding a dependency on `commons-io`, that's been discussed in the past, but the decision at that time was to not add the dependency, to keep `tika-core` as light-weight as possible. I'd take a look at the mailing list archives to get the full context of that discussion (I think there might also be Jira issues), if you want to revisit it.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

PeterAlfredLee commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-713233915


   > TaggedIOException can just extend IOException, yes?
   
   Yes, it is.
   
   > I'd take a look at the mailing list archives to get the full context of that discussion (I think there might also be Jira issues), if you want to revisit it.
   
   Maybe you are talking about [TIKA-1706](https://issues.apache.org/jira/browse/TIKA-1706) and [TIKA-1710](https://issues.apache.org/jira/browse/TIKA-1710)? Seems that it's agreed to have `commons-io` as a dependency of `tika-core`. Are there any other mailing threads or issues? It's appreciated if you can find them. :)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] tballison commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

tballison commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-713586085


   My memory was that we had agreed to add `commons-io` to tika-core in 2.x, and Bob already did that in the 2.x branch, I haven't had a chance to port those updates to `main` yet.
   
   I, frankly, want to keep some subclass of IOException around whether that's IOExceptionWithCause or something else.  My reasoning is that we have to wrap SAXExceptions and TikaExceptions in IOExceptions because of the exception signatures in some of our overridden classes.  It is useful to be able differentiate this hack from an actual IOException.
   
   Is there a better way to achieve this goal?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

PeterAlfredLee commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-714174957


   > My memory was that we had agreed to add commons-io to tika-core in 2.x, and Bob already did that in the 2.x branch, I haven't had a chance to port those updates to main yet.
   
   I see. I'm almost finished with the code and tests. Maybe I can push a PR to `main` branch?
   
   > Is there a better way to achieve this goal?
   
   Seems we do not have any other ways to do that. Maybe you have any better ideas?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

kkrugler commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-722693051


   Hi @tballison - you said:
   
   > I, frankly, want to keep some subclass of IOException around whether that's IOExceptionWithCause or something else. My reasoning is that we have to wrap SAXExceptions and TikaExceptions in IOExceptions because of the exception signatures in some of our overridden classes. It is useful to be able differentiate this hack from an actual IOException.
   
   Couldn't we have a helper class that checks for `IOException.getCause()` being an instance of those two exceptions? I'm asking because I also run across a lot of cases in other projects where the root cause has to be wrapped in an `IOException`, but in the very rare case where I care about what really caused the problem, I call `getCause()`. But maybe there's something different about this particular use case.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

kkrugler commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-731297780


   Hi @tballison - hoping you can weigh in on whether we really need to keep around `IOExceptionWithCause`, thanks.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] tballison commented on pull request #369: Use IOException instead of IOExceptionWithCause

GitBox
In reply to this post by GitBox

tballison commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-731300730


   Sorry for my delay.  I'm now +1 to this.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]