ppkarwasz commented on PR #784: URL: https://github.com/apache/commons-io/pull/784#issuecomment-3322986322
Hi @garydgregory, > This is definitely not a bug and is the expected behavior. A couple of points: > > * Maybe the type-specific origin classes like `InputStreamOrigin` shouldn't have been public, but it would be harder to provide new conversions outside the library itself, but maybe IO isn't used at that level of depth. A lesson learned for the future. > > * The type-specific origin classes (here, `InputStreamOriogin`) are all _wrappers_ of that specific type, and always return the instance it was given on creation from its same-type getter (here, `getInputStream()`). In this case, an `InputStreamOrigin` is constructed with an `InputStream` and always returns that `InputStream` from `getInputStream()` > > * The previous point highlights the need for better Javadocs I think. Thanks for the clarification. If returning the **same instance** from type-specific origins is the intended behavior, we should make that explicit: right now callers can’t know whether `AbstractOrigin#getInputStream()` returns a fresh stream or a wrapper over an already-open resource, so they’ll (correctly) close it. So the Javadoc should say something like: > Callers **own** the returned stream and **must** close it. Closing the returned stream closes the underlying resource. If you need a close-shielded view, wrap the stream before returning it. Note that we should adapt the tests to also **close** the resources hold by the origin. After moving the read-write origin to a temporary per-test folder, I got some errors on Windows due to unclosed resources: https://github.com/apache/commons-io/pull/784/commits/1fc9097f3c3bd5ac3223bd91fdf265c9390a3f7b. > > I’m not strongly against exposing a public `ByteArraySeekableByteChannel(byte[])` constructor, though I personally prefer the `wrap` factory method. > > Would you please explain why one versus the other? I personally prefer a factory method (`wrap(byte[])`) over a public constructor because it allows validation, `import static` and future flexibility (e.g., caching, different implementations) without breaking API. However, in this case I don't see any possible future changes. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
