You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/06/01 16:46:19 UTC
[GitHub] [commons-io] adamretter opened a new pull request #119: Add a MarkShieldInputStream
adamretter opened a new pull request #119:
URL: https://github.com/apache/commons-io/pull/119
This is similar to the `CloseShieldInputStream` but it focuses on the mark, markSupported, and reset methods.
----------------------------------------------------------------
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] [commons-io] garydgregory merged pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #119:
URL: https://github.com/apache/commons-io/pull/119
----------------------------------------------------------------
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] [commons-io] garydgregory commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-642752890
> @garydgregory If you look at `java.io.InputStream` source code - the default is to throw an exception when reset is called if mark is unsupported... so I am just following the defacto Java standard.
Yes, but `InputStream` is an abstract class, so there are no instances of it flying around. Why should this class blow up that call instead of passing it on? Or, does it make sense to have this class shield _both_ `mark()` _and_ `reset()`?
----------------------------------------------------------------
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] [commons-io] adamretter commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
adamretter commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-636984631
@garydgregory Hmm I had not thought of that. Personally I prefer to have the separate classes... but I can refactor it if that's what is needed?
What would happen to the existing `CloseShieldInputStream`? Would it just be removed?
----------------------------------------------------------------
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] [commons-io] adamretter edited a comment on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
adamretter edited a comment on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-637099087
@garydgregory If you look at `java.io.InputStream` source code - the default is to throw an exception when reset is called if mark is unsupported... so I am just following the defacto Java standard.
----------------------------------------------------------------
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] [commons-io] adamretter commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
adamretter commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-637099087
@garydgregory If you look at java.io.InputStream source code - the default is to throw an exception when reset is called if mark is unsupported... so I am just following the defacto Java standard.
----------------------------------------------------------------
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] [commons-io] adamretter commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
adamretter commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-639768421
@garydgregory Could I get an update on this please?
----------------------------------------------------------------
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] [commons-io] adamretter edited a comment on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
adamretter edited a comment on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-636984631
@garydgregory Hmm I had not thought of that. Personally I prefer to have the separate classes... but I can refactor it if that's what is needed?
What would happen to the existing `CloseShieldInputStream`? Would it just be removed? I think that could break a lot of existing code!
----------------------------------------------------------------
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] [commons-io] garydgregory commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-637090936
Yeah, this gets tricky and arbitrary: the class is called "MarkShield" which reimplements `mark()` as a noop but it also reimplements `reset()` to throw an `IOException`, which feels like a surprise to me (not in a good way.) Why should one method be a noop and the other throw an exception? It seems to be they both should be noops. 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] [commons-io] garydgregory commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-636991133
We work hard to avoid breaking binary compatibility. Instead, we mark code as deprecated and that code can then be considered for removal for the next major version, which would include a package name change along with a matching Maven coordinate change.
Let me poke around later...
----------------------------------------------------------------
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] [commons-io] coveralls commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-636994860
[](https://coveralls.io/builds/31165252)
Coverage increased (+0.07%) to 89.791% when pulling **dc199b473acc6766dcad769ad267ec0eb627e61c on adamretter:feature/mark-shield-input-stream** into **cd7787277b170d5f439df03850ca0c574566e539 on apache:master**.
----------------------------------------------------------------
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] [commons-io] garydgregory commented on pull request #119: Add a MarkShieldInputStream
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #119:
URL: https://github.com/apache/commons-io/pull/119#issuecomment-636981406
Before we need one more and one more shield types of ProxyInputStreams, shouldn't we consider a more general shield class that takes an enum {CLOSE, MARK, THIS, THAT} so we can just have a single one?
----------------------------------------------------------------
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