You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tika.apache.org by GitBox <gi...@apache.org> on 2020/10/19 09:09:22 UTC
[GitHub] [tika] PeterAlfredLee opened a new pull request #369: Use IOException instead of IOExceptionWithCause
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:
users@infra.apache.org
[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee edited a comment on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee edited a comment on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-751912397
> that you would delete the IOExceptionWithCause class. Did that happen separately?
Yes, this is already done in #370.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
kkrugler commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-751746256
@PeterAlfredLee - I thought, based on @tballison input above, that you would delete the `IOExceptionWithCause` class. Did that happen separately?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] tballison commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-751546770
Hey folks. I'm thinking about merging this before the release of Tika 2.0. 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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] tballison commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-751912397
> that you would delete the IOExceptionWithCause class. Did that happen separately?
Well, this is already finished in #370.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee merged pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee merged pull request #369:
URL: https://github.com/apache/tika/pull/369
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee edited a comment on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee edited a comment on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-751912397
> that you would delete the IOExceptionWithCause class. Did that happen separately?
Yes, this is already finished in #370.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [tika] kkrugler commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] PeterAlfredLee commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [tika] tballison commented on pull request #369: Use IOException instead of IOExceptionWithCause
Posted by GitBox <gi...@apache.org>.
tballison commented on pull request #369:
URL: https://github.com/apache/tika/pull/369#issuecomment-751723128
+1
----------------------------------------------------------------
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:
users@infra.apache.org