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

Update README.md #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update README.md #20

wants to merge 3 commits into from

Conversation

blutack
Copy link

@blutack blutack commented Jan 19, 2018

Consider updating the readme to include a roundtrip through the serializer? Made it a bit clearer to me how the library works.
The only ugly bit is having to seek the IO::Memory back to position 0, maybe there is some way to get round that which someone better at Crystal knows.

Update the example to perform a roundtrip through the serializer and show the results
@jeromegn
Copy link
Owner

Oh, interesting.

The way you've done it certainly allows people to try it locally more easily. However, the point of protobuf is transferring it or storing it somewhere. The current usage example was aimed more at real-world usage.

The comment # get your IO in some way should've been enough to explain you need somewhere to read protobuf from. You're right, it should be explained better. I think it's good enough for people used to protobuf or using IOs, but probably not for people starting.

I think your changes may better be suited in an examples/ directory though. The awkward seeking does not serve the purpose of the readme too well. I think it may create more confusion.

Maybe a 2-step usage section would work better?

  • Produce protobuf-encoded data, store in a file (but this would normally be sent over the network or stored somewhere else where it makes more sense)
  • Consume protobuf from the stored file (via File.read or File.open)

What do you think?

@blutack
Copy link
Author

blutack commented Jan 22, 2018

As you say, the # get your IO should make it clear and the current example demonstrates perfectly well the API surface. The only reason I suggested the change was to make it clear that MyMessage behaves as a completely normal struct, and so that if you paste the example straight into a file it will run. Totally agree with you on the seek - it's an ugly implementation detail.

I've updated the example to roundtrip through a tempfile - though possibly a normal file would be cleaner? This way avoids dropping a file on the user's machine at the expense of an extra require. Regardless, thanks for the awesome library and I won't be offended if you'd prefer to keep the example the way it is currently!

Use a file for roundtripping rather than an IO::Memory
Actually require "tempfile"
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