garydgregory edited a comment on pull request #301: URL: https://github.com/apache/commons-io/pull/301#issuecomment-967099141
Ah, a great topic of discussion here :-) There are a few issues to address. TLDR for me is -1. First, something is off with the content of this PR: No tests! While this PR is green, the tests pass and BC seems preserved, it does not prove what it claims to do, specifically: "to allow access to the methods isCauseOf and throwIfCauseOf". Where are the tests that prove this to be true? Not in this PR ;-) We can "see" that by changing the class being extended this _should_ be true, but it is not _proven_ by the fact that we have _new_ tests that exercise these desired APIs and show that tests _compile_ and _pass_. The second issue is related to maintenance: The changes add complexity to some of the code and now require try-catch clauses in SwappedDataInputStream and TeeInputStream. I am saying this as an observation, not a necessarily a criticism ;-) What is not clear to me is if the implementation is even correct: Why catch IOException and not Exception? Consider streams that throw exceptions like IllegalArgumentException, IllegalStateException, and so on. The final and more important issue is about design: Using and developing streams in Java favor composition instead of extension. This PR goes the extension route and I'm not sure that is the best. My question is: As a user, can I achieve the same goal by composing my streams without this PR? If you can show that this cannot be done, then we can consider what to do about the other two issues I present above. We should perhaps discuss each class one at a time WRT composition vs. extension to ensure we do not miss anything subtle. Changing a type hierarchy should be done carefully and with purpose, not just because we can. To this point, one last but important question: What is the motivation here? Do you need this feature because it cannot be done with composition, or you do not want to do it with composition? A possible final nail in the coffin for me is that this PR goes against the intent expressed as in the Javadoc of the TaggedInputStream class (my bold): "An input stream **decorator** that tags potential exceptions so that...". I am curious as to what you all think. -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
