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 #61: implement decodeDelimitedMessageH (decoding from a file handle) #324

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@ulysses4ever
Copy link
Contributor

commented Jun 9, 2019

No description provided.

@googlebot

This comment has been minimized.

Copy link

commented Jun 9, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Jun 9, 2019
@judah

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2019

Thank you for the patch! I'll take a look once the CLA is signed. Let me know if you have any questions about getting it working.

@mboes

This comment has been minimized.

Copy link

commented Jun 10, 2019

@ulysses4ever as been added to Tweag's google-contributors@ whitelist. So he has transitively signed the CLA.

@googlebot

This comment has been minimized.

Copy link

commented Jun 10, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 10, 2019
Copy link
Collaborator

left a comment

Thank you for the patch! The overall approach looks reasonable; I've suggested some minor changes.

getVarIntH h = do
buf <- malloc
let loopStart !s !n = do
_ <- hGetBuf h buf 1

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

You should check the result of hGetBuf. It might return a value of zero at EOF, in which case b would be undefined. At worst case, that would mean this could loop forever.

This comment has been minimized.

Copy link
@ulysses4ever

ulysses4ever Jun 10, 2019

Author Contributor

Good point! What would be a reasonable way to fail gracefully in case hGetBuf returns 0?

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

Hm, after thinking about it: since the function that ultimately calls this one returns IO (Either String ...), you should probably return a similar type from this function: IO (Either String Word64).

Maybe it would make sense to simplify the internal error-passing via ExceptT String IO. Up to you.

@@ -209,6 +209,17 @@ tests:
- Proto.Any
- Proto.Any_Fields

decode_delimited:

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

Nit: please follow the convention of the other tests and name this "decode_delimited", and name the corresponding source file "decode_delimited_test.hs".

This comment has been minimized.

Copy link
@ulysses4ever

ulysses4ever Jun 10, 2019

Author Contributor

Sorry, I didn't get: is the name of the test wrong too? The name of the file I will fix, thanks!

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

Yeah, by convention the test names in package.yaml also have "_test" at the end.

h <- openBinaryFile filename ReadMode
any1 <- decodeDelimitedMessageH h :: IO (Either String Foo)
hClose h
removeIfExists filename

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

When would we not expect the file to exist? Would it make the test simpler/more robust if we just threw an error if the file unintentionally disappeared?

This comment has been minimized.

Copy link
@ulysses4ever

ulysses4ever Jun 10, 2019

Author Contributor

That makes sense. Do you want me to leave just the call to removeFile (which might throw) without additional bookkeeping?

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

withSystemTempFile should remove the file automatically, so I believe you could just delete the call.

[ testCase "buildDelimited/decodeDelimited" $ do
let foo = defMessage & a .~ 42 & b .~ "hello" :: Foo
let bs = runBuilder . buildMessageDelimited $ foo
B.writeFile filename bs

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

Let's use the temporary package to avoid writing files in the current directory, which would be the proto-lens-tests source directory in this case.
http://hackage.haskell.org/package/temporary-1.3/docs/System-IO-Temp.html#v:withSystemTempFile

@@ -7,8 +7,11 @@ module Data.ProtoLens.Encoding (
-- ** Delimited messages
buildMessageDelimited,
parseMessageDelimited,
decodeDelimitedMessageH,

This comment has been minimized.

Copy link
@judah

judah Jun 10, 2019

Collaborator

I think we should try to mirror the naming scheme of the other functions:

decodeMessage, decodeMessageDelimited, decodeMessageDelimitedH.

Does that sound reasonable to you?

This comment has been minimized.

Copy link
@ulysses4ever

ulysses4ever Jun 10, 2019

Author Contributor

Ouch, silly me! I can't believe I got this wrong. Will fix.

@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

I tried to address all your comments, thanks a lot for reviewing! Waiting for more comments!

@@ -47,3 +52,10 @@ parseMessageDelimited = do
len <- Bytes.getVarInt
bytes <- Bytes.getBytes $ fromIntegral len
either fail return $ decodeMessage bytes

-- | Same as @decodeMessage@ but for delimited messages read through a Handle
decodeMessageDelimitedH :: Message msg => Handle -> ExceptT String IO msg

This comment has been minimized.

Copy link
@judah

judah Jun 13, 2019

Collaborator

Hm, my original suggestion had been to keep ExceptT internal to the implementation, and still return IO (Either String msg) from this function.

Now that I see this, I'm on the fence which way is better, but leaning towards plain IO. I think ExceptT could make sense if we had several different functions we wanted to compose, but I'm not sure we'll add anything beyond this one.

What do you think? Would ExceptT simplify the code you plan to use this with, or would you end up just unwrapping it with runExceptT when you used it?

This comment has been minimized.

Copy link
@ulysses4ever

ulysses4ever Jun 13, 2019

Author Contributor

I agree with you: returning IO of Either feels more natural in the current interface than returning ExceptT. Let me implement that.

This comment has been minimized.

Copy link
@ulysses4ever

ulysses4ever Jun 13, 2019

Author Contributor

Done.

@judah
judah approved these changes Jun 14, 2019
@judah judah merged commit 5eb4ea1 into google:master Jun 14, 2019
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Thanks a lot! I'd appreciate if you could release it any time soon (but no rush, of course).

@ulysses4ever ulysses4ever deleted the tweag:add-decode-delimited-handle branch Jun 14, 2019
@judah

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2019

@ulysses4ever I've released proto-lens-0.5.1.0 containing this change.

@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@judah thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.