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

Reader.String()/Bytes()/Int() should return an error if Peek() fails rather than destroy the connection state #15

Open
mikn opened this issue Aug 24, 2020 · 1 comment
Labels
bug Something isn't working

Comments

@mikn
Copy link
Owner

mikn commented Aug 24, 2020

Reader functions String() and Bytes() and thus Result.Scan(Scanner) currently only supports returning values when tokenLen < buf.Size() the problem is obviously that this is a bit obscure. Should this be documented as an "expected behaviour" and steer people to use Result.Scan(io.Writer) instead (and throw appropriate error messages) or should we implement support for size up to Redis max size?
The first way would encourage "good behaviour", as in that it makes people think about how much memory they're allocating (and thus copying) and steer them to using various buffers (such as strings.Builder{} or bytes.Buffer{}) rather than "blindly" use the primitive types. The second would allow people to shoot themselves in the foot if they want to, and many people may want that. 🤔

I'm currently leaning towards throwing errors and encourage good behaviour as I'm a bit of a Good Samaritan in that sense.

@mikn mikn added the question Further information is requested label Aug 24, 2020
@mikn mikn changed the title Should Reader.String() etc (implemented through Peek) support "unlimited size"? Should Reader.String() etc support "unlimited size"? Aug 24, 2020
@mikn mikn added bug Something isn't working and removed question Further information is requested labels Apr 21, 2021
@mikn
Copy link
Owner Author

mikn commented Apr 21, 2021

Currently the code implements neither approach and would permanently mess up the state of the connection if it were to encounter a situation where it couldn't call Peek() to read the full token. With the redesign of the internals, surfacing a relevant error should be significantly easier.

@mikn mikn changed the title Should Reader.String() etc support "unlimited size"? Reader.String()/Bytes()/Int() should return an error if Peek() fails rather than destroy the connection state Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant