Skip to content

Conversation

c3-robin
Copy link
Contributor

@c3-robin c3-robin commented Aug 13, 2024

This causes the only byte in the file to be lost. For this issue #4

@HarryWeppner
Copy link

@guss77 is this a PR you could help review and create a new hotfix release once merged? Thanks!

@guss77
Copy link
Contributor

guss77 commented Aug 14, 2024

@c3-robin - on the face of it this looks like a working solution(*). Can you also add a test that demonstrates that the problem has been resolved and protect against regressions?

*) I think that I'm not happy about the recursion anyway and maybe there's a better way to solve the problem by unwinding the recursion.

@guss77 guss77 merged commit 41e1f5b into cloudonix:main Aug 14, 2024
@guss77
Copy link
Contributor

guss77 commented Aug 14, 2024

@c3-robin thank you for your contribution - I accepted the PR and the problem is now fixed in main.

I went ahead and wrote the unit test for that, mostly because I thought I can fix this another way and also get rid of the recursion, but this proved to be way too much code and not worth the trouble, and your simple solution is probably best.

Thank you!

(@HarryWeppner )

@c3-robin
Copy link
Contributor Author

@c3-robin - on the face of it this looks like a working solution(*). Can you also add a test that demonstrates that the problem has been resolved and protect against regressions?

*) I think that I'm not happy about the recursion anyway and maybe there's a better way to solve the problem by unwinding the recursion.

Thanks for adding the test and bringing in the fix! I saw the original InputStream implementation of read did not use recursion as well, but noticed recursion was theoretically correct and had much cleaner code.

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.

3 participants