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

fix incorrect end of file checking and add some unit tests #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

panikgaul
Copy link

Test run output:

$ go test  -covermode=count  ./buffe
ok  	github.com/xitongsys/parquet-go-source/buffer	(cached)	coverage: 100.0% of statements

@@ -71,7 +73,8 @@ func (bf *BufferFile) Read(p []byte) (n int, err error) {
n = copy(p, bf.buff[bf.loc:len(bf.buff)])
bf.loc += n

if bf.loc == len(bf.buff) {
// if copied files into buffer with not enaugh capacity
if bf.loc < len(bf.buff) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@panikgaul 100% unit test coverage is nice.

But, it seems that your change is wrong and the code before the change is correct.

Did you intended if n < len(p) { ?

Even so, I don't understand how io.EOF relates to the capacity of the destination buffer.

Copy link
Author

Choose a reason for hiding this comment

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

Hello,

yes the if n < len(p) { was my intention.

When I used this with ==, I always got EOF, but that is not correct.
If destination buffer is smaller then source buffer, copy() gives you the count of copied bytes -> size of destination bugger, and you should get EOF in this case, because the source was not copied completely. That was my understanding and why I switched to <.
Now it works as expected.

I hope it was plausible :)

Copy link
Contributor

Choose a reason for hiding this comment

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

A partial copy due to insufficient capacity in the destination buffer (even if nothing is copied to a destination buffer with a capacity of 0 bytes) is not an error.

The io.EOF only indicates whether the end of the source has been reached.
It is absolutely not related at all with the destination buffer.

It is correct behavior for the Read of io.Reader to return io.EOF when the source is fully read.

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