Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stream locations #340

Merged
merged 6 commits into from
Oct 16, 2018
Merged

Add stream locations #340

merged 6 commits into from
Oct 16, 2018

Conversation

gab1one
Copy link
Contributor

@gab1one gab1one commented Oct 15, 2018

These are currently used for Compressed input streams #274 and https://github.com/scijava/scijava-io-http

@gab1one gab1one requested a review from ctrueden October 15, 2018 13:55
@ctrueden ctrueden force-pushed the stream-locations branch 2 times, most recently from d6ecb02 to 082fc60 Compare October 15, 2018 14:17
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gab1one! This is looking good.

There are/were three problems, the first two of which I already fixed:

  1. Misspelling of cutoff (only has one T).
  2. Compile errors with DefaultBufferedStreamHandle calling ByteBank.getMaxPos() (it was changed to ByteBank.size()), and missing exists() method implementation.
  3. No unit tests for DefaultBufferedStreamHandle.

For (1) and (2), I fixed and force pushed the relevant commits.

For (3), could you please add a test?

The findString(..) method used to require the exact length of the
handle, this length can be unknown, e.g. for compressed handles.
This is	no longer the case.
@gab1one
Copy link
Contributor Author

gab1one commented Oct 15, 2018

Thanks for fixing this,will add the Test ASAP

This basic version does not fully support random access, which will be
provided by subclasses to be added in future commits.
These subinterfaces of StreamHandle provide random access over the
contained streams.
@gab1one
Copy link
Contributor Author

gab1one commented Oct 16, 2018

I am actually not sure if we need the DefaultBufferedStreamHandle at all, I implemented a test and discovered a series of issues with it's design. I think the ReadBufferDataHandle / WriteBufferDataHandle are better and fulfill the same role.

@gab1one gab1one force-pushed the stream-locations branch 2 times, most recently from 9de55e6 to 8376a59 Compare October 16, 2018 14:53
Some handles don't know their length and instead return -1, this commit
adds an option to the length check to assure this.
@ctrueden ctrueden merged commit b25bd7c into master Oct 16, 2018
@ctrueden ctrueden deleted the stream-locations branch October 16, 2018 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants